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

Allow event fetched on diffs#475 #1224

Closed

Conversation

@einarhuseby
Copy link
Contributor

@einarhuseby einarhuseby commented Jan 27, 2019

See PR #1223 for the other alternative introducing on_fetched_diffs.

This PR allows version=diffs to run when on_fetched* event hooks are wired up.

This makes all item and resource fetches fire the callback. A use case could be anonymizing documents via on_fetched_*. Accessing with version=diffs would expose the documents.

As discussed in #475 and inline code notes version=diffs will return partial documents which can have unexpected results if it's not handled correctly in the callback.

@nicolaiarocci nicolaiarocci added this to the 0.8.2 milestone Jan 28, 2019
einarhuseby added 2 commits Jan 28, 2019
Feature new event hook for diffs, merged branch instead of new PR to pyeve/eve
@einarhuseby
Copy link
Contributor Author

@einarhuseby einarhuseby commented Jan 28, 2019

Merged the changes as discussed in #1223 to this PR instead of making another PR since this is labelled for 0.8.2.

The callback now gets the entire list of diffs, which I think makes sense. Also updated the tests in TestCompleteVersioning; test_on_fetched_diffs and test_on_fetched_diffs_contacts.

Hateos _links seems to get an extra &version=diffs but I haven't looked further into that. Not sure if it's caused by this PR.

@nicolaiarocci
Copy link
Member

@nicolaiarocci nicolaiarocci commented Jan 29, 2019

Hateos _links seems to get an extra &version=diffs but I haven't looked further into that. Not sure if it's caused by this PR.

Can you please investigate further on this, and report back. Thanks.

@einarhuseby
Copy link
Contributor Author

@einarhuseby einarhuseby commented Jan 29, 2019

Looks like default_params is missing version

eve/eve/methods/get.py

Lines 622 to 634 in 7a8a6c2

def _other_params(args):
""" Returns a multidict of params that are not used internally by Eve.
:param args: multidict containing the request parameters
"""
default_params = [
config.QUERY_WHERE,
config.QUERY_SORT,
config.QUERY_PAGE,
config.QUERY_MAX_RESULTS,
config.QUERY_EMBEDDED,
config.QUERY_PROJECTION,
]
and then it gets added as other_params in
other_params_part = "".join(

Only observed for _links['self']['href']

Also, maybe QUERY_VERSION instead of VERSION_PARAM would make sense?

nicolaiarocci added a commit that referenced this pull request Feb 9, 2019
@nicolaiarocci
Copy link
Member

@nicolaiarocci nicolaiarocci commented Feb 9, 2019

rebased and merged, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants