-
Notifications
You must be signed in to change notification settings - Fork 6
Version 1 #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Version 1 #5
Conversation
alexandros-georgantas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Evaki 👋
Thank you for your work on this assignment!
Overall, your submission addresses some aspects of the task, though the solutions lean towards the simpler side. I sensed that our deadline might have been tight for you, and perhaps with more time, the outcome would have been different.
It's perfectly fine that some elements were missed.
I've left some comments on your Pull Request with questions. I'd be happy to hear your answers if you're inclined to provide them.
Thanks,
Alex
| @@ -0,0 +1,33 @@ | |||
| db.createUser( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given more time, how would you have refactored this schema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
I would have probably added stricter validation rules and indexing like so:
-
add pattern validation for uuids in session_id field which will ensure uniqueness of ids and can be easily generated using libraries (most languages provide such libraries)
-
add validation for the messages array structure:
- set an enum for the role field enum: ["question", "answer"]
-
add a maxItems field to the messages array, so that there are no errors or performance issues produced from large arrays (as this application might only need to store a limited number of recent messages for a session I would set it to 100)
-
add min and max length for text (minLength: 1, maxLength: 1000) which would prevents the creation of excessively large strings that could impact performance or storage
-
add indexing by sessionId for faster search performance (db.userhistory.createIndex({ sessionId: 1 });)
| @@ -0,0 +1,147 @@ | |||
| package controller | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please describe what would have been your approach in order to test the functionality of this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(First of all, I believe that the function that calls the other API should be moved to a different file and not sit in the controller file)
As a first approach, I would use mock libraries to mock the calls to the python API and the database operations (possibly this one: github.com/stretchr/testify/mock). That way, I can test the controller logic independently of dependencies. The test cases would roughly cover the following:
- request validation
- errors from request validations
- error handling from database errors
- error handling from empty query response from database
- error handling from python api call errors
- error handling of empty response from python API
- empty message inputs
- very long message inputs
- unusual messages (check user input is properly sanitizes)
| @@ -0,0 +1,29 @@ | |||
| package model | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you had time, what other entities you would have created to meet the requirements of the assignment (e.g. feedback aspect)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requirements of the assignment, based on the readme file and my understanding, was to create chat bot endpoint/web service that:
- uses the data provided to give users replies (DONE - python API)
- persists the chat data (DONE - stored in mongo db)
- continue the conversation where it was left off
- user opens multiple chats (DONE - use of session Ids)
For the third bullet point I would have created an endpoint that returns the user's conversation.
As for the optional enhancements which included the feedback aspect, I would have used the same post endpoint and store the feedback in the database in the messages list with an extra role called "feedback" that would have a positive or negative value and would include an optional comment from the user.
| fmt.Println(uri) | ||
|
|
||
| // Initialize global cache | ||
| GlobalCache, err = bigcache.NewBigCache(bigcache.DefaultConfig(30 * time.Minute)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've initialized a cache. How is it being utilized within your code? Please explain your decision to include a caching layer and its intended use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The structure for my go app was derived from Rohit Binjola's example app (the link can be found in the top of the main.go app). They added the cache capability to their database module. I did not end up using it, but I could possibly use it in the database handler if I were, for example, to fetch the session data from the database based on specific session ids.
| resource := router.Group("/api") | ||
| resource.Use(middleware.LogRequestInfo()) | ||
| { | ||
| resource.POST("/question", controller.SubmitQuestion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you had more time what other endpoints you would have created to meet the assignment's requirements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same answer: #5 (comment)
|
|
||
| func NewRouter() *gin.Engine { | ||
| router := gin.New() | ||
| resource := router.Group("/api") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please provide a suggestion on how this could be refactored to support potential breaking changes of the API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use versioning by creating different paths /api/vi, where i is the version of the api. That will allow us to change the behavior or structure of your API without affecting users.
dzacharakis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for your submission!
We really appreciate the time and effort you put into this. I have a few questions I’d love your thoughts on—could you take a look when you get a chance?
Thanks
| ### 🐳 Service Description | ||
| #### 1. Question API (Go) | ||
|
|
||
| Port: 8080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: config and compose expose 7004
|
|
||
| response, err := PostJSON[RequestPayload, model.SubmitQuestionRequest](url, req) | ||
| if err != nil { | ||
| log.Fatalf("Fatal error calling matching API: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn’t it be better to use log.Errorf instead and why?
| Score float64 `json:"score"` | ||
| } | ||
|
|
||
| func RemoveConversation(c *gin.Context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that RemoveConversation exists but isn’t registered in the router. Leftover?
|
|
||
| func Init() { | ||
| router := NewRouter() | ||
| router.Run(config.Appconfig.GetString("server.port")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error return value of router.Run is not checked.
| router.Run(config.Appconfig.GetString("server.port")) | |
| err := router.Run(config.Appconfig.GetString("server.port")) | |
| if err != nil { | |
| log.Fatalf("Failed to start server: %v", err) | |
| } |
Any possible tool that could be helpful here?
| } | ||
|
|
||
| func NewRouter() *gin.Engine { | ||
| router := gin.New() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm expecting the server would crash on an internal error. If you want it to keep running as expected, what approaches could you take? Two come to my mind—what might Gin offer that you may have missed?
| rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8= | ||
| rsc.io/pdf v0.1.1/go.mod h1:n8OzWcQ6Sp37PL01nO98y4iUCRdTGarVfzxY20ICaU4= | ||
| rsc.io/quote/v3 v3.1.0/go.mod h1:yEA65RcK8LyAZtP9Kv3t0HmxON59tX3rD+tICJqUlj0= | ||
| rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this go.sum file has 650 lines of packages. Have you considered module maintenance—like cleaning up/removing unused modules?
| req.Header.Set("Content-Type", "application/json") | ||
|
|
||
| // Send the request | ||
| resp, err := client.Do(req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to also check resp.StatusCode here before JSON unmarshal?
| } | ||
|
|
||
| // PostJSON sends a POST request with JSON-encoded data and decodes the JSON response. | ||
| func PostJSON[T any, R any](url string, payload model.SubmitQuestionRequest) (*ResponsePayload, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are generics used here if neither type parameter (T, R) is actually utilized? As it stands, this looks like a normal function with unused type parameters.
This is my attempt for a solution to the challenge. I do not have any experience in go but focused on creating a tidyish structure with the go api. As for the python bit it was created fast and is a bit untidy and some bits do not work. Given the tight timeframe I think I did what I could. Any feedback is, of course, welcome. Thank you!