Fetch Workflows #56
Fetch Workflows #56
Conversation
kartikay101
commented
Oct 12, 2020
kartikay101
commented
utils/parser.go
Outdated
| } | ||
|
|
||
| func cleanFilters(filters map[string]string) (map[string]string, error) { | ||
| supportedKeys := []string{"id", "createdDate", "name"} |
There was a problem hiding this comment.
Magic strings should be in constants. When a string is being used in multiple places, pull them out into constants so that change in string value is isolated to a variable
There was a problem hiding this comment.
If you could suggest a proper place for it. That would be great
repository/postgres.go
Outdated
| query := p.getDb().Model(&pgWorkflows) | ||
| for key, value := range filters { | ||
| if value != "" { | ||
| query = query.Order(key + " " + value) |
There was a problem hiding this comment.
Read about sql injection and see how you can prevent it in go lang? Are we susceptible to SQL injection here? Can we validate?
There was a problem hiding this comment.
you are using filters for ordering? Shouldn't filters be used for filtering and then sortBy query params for ordering? This doesn't feel right.
There was a problem hiding this comment.
I was thinking about not calling it filters. I will rename it to something else.
As far as SQL injection is concerned, the parser ensures no illegal filters passed
utils/parser.go
Outdated
| @@ -0,0 +1,46 @@ | |||
| package utils | |||
There was a problem hiding this comment.
I don't like utils as a package name. I would rather do filtering or filter as package and then parsing and other functions will be in that package. utils means that I could not identify a domain object for this responsibility so lets use utils.
There was a problem hiding this comment.
This can be changed I think filters is fine
There was a problem hiding this comment.
Filters is not the right name if this is sorting criteria. It should be order or ordering package instead.
There was a problem hiding this comment.
Ok I didn't realize that utils is outside repository. This is a bad design. The db or repository related stuff should be inside repository package. And any filtering or sorting should be within postgres file since if we decide to have multiple repositories the implementation of repository will differ. Lets talk about this one.
handlers/workflow_handler_test.go
Outdated
| @@ -250,3 +251,26 @@ func TestShouldThrowErrorIfQueryParamsAreNotValidValuesInGetAllWorkflows(t *test | |||
| assert.NotNil(t, jsonResp) | |||
| assert.Equal(t, "page number or page size is not in proper format", jsonResp.Message) | |||
| } | |||
|
|
|||
| func TestShouldThrowErrorIfFiltersAreNotInTheRightFormat(t *testing.T) { | |||
There was a problem hiding this comment.
This is an integration test. These are expensive to run and manage. Here you have only covered negative path. If anything you should have a happy path at least.
There was a problem hiding this comment.
Alternatively, extracting error handling code into a common method and writing tests only once for that is better. All positive and negative tests can be covered in actual implementations' unit test
utils/parser.go
Outdated
| cleanedFilters := make(map[string]string) | ||
| for _, key := range supportedKeys { | ||
| value := filters[key] | ||
| if value != "asc" && value != "desc" && value != "" { |
There was a problem hiding this comment.
Should it be case insensitive?
There was a problem hiding this comment.
What happens down the method if the string is empty? Are you setting a default? It doesn't look like
There was a problem hiding this comment.
yes it can be case insensitive ( do we want it to be ?)
Go by default assigns "" to keys that are missing so just to verify validity we need to check empty too
There was a problem hiding this comment.
but for keys that are valid and value is empty what happens? Also, yes if DB support case insensitivity, which I think it does, it should be. Or we should force lowercase conversion before parsing the string.
