Ensure that DJORNL queries do not have additional properties#26
Ensure that DJORNL queries do not have additional properties#26jayrbolton merged 3 commits intodevelopfrom
Conversation
Add "additionalProperties" to stored queries
| has_ws_ids = False | ||
| if 'ws_ids' in stored_query['query']: | ||
| json_body['ws_ids'] = ws_ids | ||
| has_ws_ids = True | ||
| else: | ||
| del json_body['ws_ids'] |
There was a problem hiding this comment.
If the stored query doesn't have ws_ids in it, the additional ws_ids param will not be needed.
There was a problem hiding this comment.
As I mentioned in this issue, I think a simpler way to achieve the same thing with less code is to just remove line 75 here: https://github.com/kbase/relation_engine/blob/develop/relation_engine_server/api_versions/api_v1.py#L75
There was a problem hiding this comment.
If the query doesn't use ws_ids, it is just potentially slowing things down by retrieving the list of IDs and adding them to the query. The cleanest solution would be to check whether the query/stored query mentions ws_ids, and only if it does, fetch the IDs from the workspace to add to the request object and alter the AQL to include the LET ws_ids = @ws_ids line.
I've reverted things to how they were, with the ws_ids injection, but moved the setting of ws_ids to after JSON validation.
There was a problem hiding this comment.
Avoiding an extra workspace request when the query does not use ws_ids is a good point, I agree that would be a good additional benefit
There was a problem hiding this comment.
I'm seeing the current version of the code doesn't try to avoid the workspace request, and also doesn't remove line 75. Maybe you haven't pushed yet?
There was a problem hiding this comment.
OK, I have edited it to only add ws_ids and pull the list from the workspace when ws_ids is mentioned in the query.
| stored_query_path = spec_loader.get_stored_query(query_name, path_only=True) | ||
| run_validator(schema_file=stored_query_path, data=json_body, validate_at='/params') |
There was a problem hiding this comment.
Use the path, so that relative references in the files work.
|
Done reviewing, one comment above |
ialarmedalien
force-pushed
the
remove_ws_ids
branch
from
c090f84 to
d308c33
Compare
ialarmedalien
force-pushed
the
remove_ws_ids
branch
from
d308c33 to
8f45ed4
Compare
| # fetch number of documents to return | ||
| batch_size = int(flask.request.args.get('batch_size', 10000)) | ||
| full_count = flask.request.args.get('full_count', False) | ||
|
|
There was a problem hiding this comment.
You don't have to revert, but i prefer not having all these additional newlines in the function body
There was a problem hiding this comment.
I find adding newlines aids readability -- otherwise it's just a bunch of lines with the same indentation and there's added cognitive load in understanding the code before you can edit it. 😖
2 participants
Disallow any additional parameters in the DJORNL queries.
ws_idsparam unless it is required by the querydistance.yamlfile for queries with a distance paramCloses #11