add dataset_ids filter #286
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gedge
reviewed
Aug 28, 2024
config/config.go
Outdated
| @@ -13,6 +13,7 @@ type Config struct { | |||
| BerlinAPIURL string `envconfig:"BERLIN_URL"` | |||
| CategoryAPIURL string `envconfig:"CATEGORY_URL"` | |||
| BindAddr string `envconfig:"BIND_ADDR"` | |||
| DebugMode bool `envconfig:"DEBUG_MODE"` | |||
There was a problem hiding this comment.
Suggested change
| DebugMode bool `envconfig:"DEBUG_MODE"` | |
| DebugMode bool `envconfig:"ENABLE_DEBUG"` |
for consistency with other boolean flags
though, I do wonder why/if this is needed at all (when would we use it in an environment? isn't this Dev-only?)
There was a problem hiding this comment.
Linden, specified that he's rather we pick it up as a config.
api/search.go
Outdated
| @@ -357,7 +484,7 @@ func LegacySearchHandlerFunc(validator QueryParamValidator, queryBuilder QueryBu | |||
|
|
|||
| nlpCriteria := getNLPCriteria(ctx, params, nlpConfig, queryBuilder, clList) | |||
|
|
|||
| q, searchReq, countReq := CreateRequests(w, req, validator, nlpCriteria) | |||
| q, searchReq, countReq := CreateRequests(w, req, nlpConfig, validator, nlpCriteria) | |||
There was a problem hiding this comment.
Suggested change
| q, searchReq, countReq := CreateRequests(w, req, nlpConfig, validator, nlpCriteria) | |
| q, searchReq, countReq := CreateRequests(w, req, cfg, validator, nlpCriteria) |
should the param to this func be renamed to be consistent?
api/search.go
Outdated
| return invalidDatasetIDs, err | ||
| } | ||
|
|
||
| func parseTopics(ctx context.Context, params url.Values) (topics []string, err string) { |
There was a problem hiding this comment.
Suggested change
| func parseTopics(ctx context.Context, params url.Values) (topics []string, err string) { | |
| func parseTopics(ctx context.Context, params url.Values) (topics []string, err error) { |
Is there a reason for using strings where one would normally/consistently return error?
These funcs seems to bounce between the two return types for a reaons that is unclear.
api/search.go
Outdated
| return sanitisedQuery, nil | ||
| } | ||
|
|
||
| func parseLimit(ctx context.Context, params url.Values, validator QueryParamValidator) (limit int, err string) { |
There was a problem hiding this comment.
Suggested change
| func parseLimit(ctx context.Context, params url.Values, validator QueryParamValidator) (limit int, err string) { | |
| func parseLimit(ctx context.Context, params url.Values, validator QueryParamValidator) (limit int, err error) { |
see later
api/search.go
Outdated
|
|
||
| if queryHasSpecialChars { | ||
| log.Info(ctx, "rejecting query as it contains special characters", log.Data{"query": sanitisedQuery}) | ||
| http.Error(w, "Invalid characters in query", http.StatusBadRequest) |
There was a problem hiding this comment.
I don't think this func - a validator - should be calling http.Error - perhaps move this back to the caller
api/search.go
Outdated
| log.Info(ctx, "rejecting query as contained special characters", log.Data{"query": sanitisedQuery}) | ||
| http.Error(w, "Invalid characters in query", http.StatusBadRequest) | ||
| // Sanitize and validate the query string | ||
| sanitisedQuery, err := sanitizeAndValidateQuery(ctx, w, params) |
There was a problem hiding this comment.
Suggested change
| sanitisedQuery, err := sanitizeAndValidateQuery(ctx, w, params) | |
| sanitisedQuery, err := sanitiseAndValidateQuery(ctx, w, params) |
inconsistent spelling (z in only 4 places)
api/search.go
Outdated
| for _, topicID := range topics { | ||
| if topicID == "" { | ||
| invalidTopics = append(invalidTopics, "<blank>") | ||
| } else if len(topicID) <= 1 || len(topicID) > 4 { |
There was a problem hiding this comment.
perhaps a comment here to explain this logic
swagger.yaml
Outdated
| @@ -71,6 +71,14 @@ paths: | |||
| type: string | |||
| collectionFormat: csv | |||
| required: false | |||
| - in: query | |||
| name: dataset_ids | |||
| description: "Comma separated list of dataset IDs to filter the results." | |||
There was a problem hiding this comment.
Suggested change
| description: "Comma separated list of dataset IDs to filter the results." | |
| description: "Comma-separated list of dataset IDs to filter the results." |
Oluwawunmi
force-pushed
the
feature/add-datasetid-filter-to-search
branch
from
a8bcb00 to
2372309
Compare
gedge
approved these changes
Aug 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
add dataset_ids filter to search api
How to review
Check ok
Who can review
Anyone