Skip to content
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

Feature/sparql bugs #481

Merged
merged 8 commits into from
Jul 6, 2021
Merged

Feature/sparql bugs #481

merged 8 commits into from
Jul 6, 2021

Conversation

JeltevanBoheemen
Copy link
Contributor

@JeltevanBoheemen JeltevanBoheemen commented Jun 22, 2021

Fixes for various SPARQL bugs (#394 #423 #469).

Clunky fix for wrong arity SELECT queries with Accept: text/turtle. See #394 (comment), let me know if you can't live with this hackiness 😄

@BeritJanssen Turns out the nlp-ontology.graph() for local testing caused most tests to fail, because it keeps resetting to the example file (was a bit of detective work to find this). Rather than try to fix that, I just moved the tests to a different endpoint.

(blank, HTTP.sc, HTTPSC.BadRequest),
)
)
renderer_context['response'].status_code = 400
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fairly hacky. Couldn't figure out how to do this more elegantly, as the error occurs very late: when the response is already formed, only the content needs to be rendered.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean it is too late to catch the exception and re-raise as a ValidationError? In that case, perhaps you can emulate that mechanism with something like the following:

# top of module
from rest_framework.exceptions import ValidationError
from ..rdf.views import custom_exception_handler

# here
         try:
             results_graph = graph_from_triples(query_results)
         except Exception as e:
             error = ValidationError(str(e))
             response = custom_exception_handler(error, renderer_context)
             results_graph = response.data
             renderer_context['response'] = response
             return super().render(results_graph, media_type, renderer_context)

Copy link
Member

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously I can't really object against a PR that fixes three bugs, but yes, the part you commented on does look a bit hacky.

backend/pytest.ini Outdated Show resolved Hide resolved
(blank, HTTP.sc, HTTPSC.BadRequest),
)
)
renderer_context['response'].status_code = 400
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean it is too late to catch the exception and re-raise as a ValidationError? In that case, perhaps you can emulate that mechanism with something like the following:

# top of module
from rest_framework.exceptions import ValidationError
from ..rdf.views import custom_exception_handler

# here
         try:
             results_graph = graph_from_triples(query_results)
         except Exception as e:
             error = ValidationError(str(e))
             response = custom_exception_handler(error, renderer_context)
             results_graph = response.data
             renderer_context['response'] = response
             return super().render(results_graph, media_type, renderer_context)

return graph.update(updatestring)
except ParseException as p_e:
graph.update(updatestring)
except (ParseException, ParseError) as p_e:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks somewhat familiar!

@JeltevanBoheemen
Copy link
Contributor Author

@jgonggrijp Thanks for the suggestion. That probably would have worked.
However, I chose a different approach. The view will have to re-negotiate to account for query type, this is 'hacky' by nature. Instead of doing this in finalize_response, it is now done in execute_query. Note that I chose finalize_response because it re-negotiates if there is no Accept provided in the request, and enforces the first available. By setting the accepted renderer in an earlier stage, this is skipped. This` fits much better in the rest_framework flow, and avoids hacky response overwrites.

response[key] = value

return response

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always nice when so much code can go!

@JeltevanBoheemen JeltevanBoheemen merged commit 2e1dc7c into develop Jul 6, 2021
@JeltevanBoheemen JeltevanBoheemen deleted the feature/sparql-bugs branch July 6, 2021 14:49
@jgonggrijp jgonggrijp added this to the Next release milestone Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants