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

Incorrect results returned from search query when logged-in #2589

Closed
judell opened this issue Oct 6, 2015 · 12 comments · Fixed by #2595
Closed

Incorrect results returned from search query when logged-in #2589

judell opened this issue Oct 6, 2015 · 12 comments · Fixed by #2595

Comments

@judell
Copy link
Contributor

judell commented Oct 6, 2015

blog.jonudell.net has 5 annotations with these group properties:

        "group": "__world__",
        "group": "__world__",
        "group": "__world__",
        "group": "MEpy2d",
        "group": "__world__",

I'm logged into 0.7.5.70 (g562d537.dirty), it reports 5 as I reckon it should because I'm a member of MEpy2d.

Badge query: GET /api/search?limit=0&uri=http://blog.jonudell.net/
Result: {"rows": [], "total": 5}

But no annotations anchor because the extension makes this query:

https://stage.hypothes.is/api/search?group=__world__&limit=200&offset=0&order=asc&sort=created&uri=http:%2F%2Fblog.jonudell.net%2F

And gets no results: {"rows": [], "total": 0}

@judell judell changed the title annotations not anchoring using stage extension 0.7.5.70 (g562d537.dirty) annotations not returned from query using stage extension 0.7.5.70 (g562d537.dirty) Oct 6, 2015
@nickstenning nickstenning changed the title annotations not returned from query using stage extension 0.7.5.70 (g562d537.dirty) Incorrect results returned from search query when logged-in Oct 7, 2015
@nickstenning
Copy link
Contributor

I think the issue here is that searches as logged-in users aren't finding all the annotations they should find. Here's a gist showing the results of identical queries to the search endpoint when logged-out vs logged-in (on stage). The logged-in query shows fewer annotations, which can't possibly be correct.

Bizarrely, removing the group=__world__ parameter from the query, when logged-in, results in the correct annotations being returned.

Either way, the invariant is this: a logged-in query should always return the same annotations or more annotations than a logged-out query. I'll look into this a bit later on today.

@seanh
Copy link
Contributor

seanh commented Oct 7, 2015

I wonder if these are pre-groups annotations that have no group field? I'll look into this anyway

@nickstenning
Copy link
Contributor

Ahhh, that might explain it. I might've been confused by the fact that they get rewritten on the way out.

@nickstenning
Copy link
Contributor

And that would explain why it works when you're logged out -- the groups feature is toggled off so the group param is ignored.

@seanh
Copy link
Contributor

seanh commented Oct 7, 2015

Oh, actually now that I look my server-side code to insert group:'__world__' if the client sends no group was one of the bits that got removed in the end, so you can totally create annotations with no group field using the API now. It shouldn't happen though as our client does send the group field, so in Jon's case these will be pre-groups annotations

@seanh
Copy link
Contributor

seanh commented Oct 7, 2015

Yep, I can reproduce this locally:

  • Use the API to create an annotation of a URI with no group field
  • Chrome extension badge counts that annotation
  • Sidebar doesn't show that annotation if groups feature is on, does show it if groups is off

@seanh
Copy link
Contributor

seanh commented Oct 7, 2015

Possible solutions:

  1. Original groups design was that annotations in the public group (i.e. in no group) would have no groups field, making them the same as pre-groups annotations and annotations created via the API without a group field. Possibly too late to go back to this now though.
  2. Have the sidebar not send any group filter when the public group is selected
  3. Have the server transform searches for the public group into searches for public or no group
  4. Put back the server-side code that silently inserts group __world__ if the client sends no group, and also do a migration for pre-group annotations
  5. As 4 but instead of inserting group __world__ just reject annotations with no group field, give a validation error

@seanh
Copy link
Contributor

seanh commented Oct 7, 2015

My vote is for 4

@nickstenning
Copy link
Contributor

Yep. 4. That requires changing only h.api.search.transform.prepare.

@seanh
Copy link
Contributor

seanh commented Oct 7, 2015

And doing an Es migration. Ok, I'll do that after lunch then

@nickstenning
Copy link
Contributor

My point was that if you change prepare, we can run a migration by doing hypothesis annotool conf/production.ini prepare.

seanh added a commit that referenced this issue Oct 7, 2015
When creating or updating an annotation if the annotation has no 'group'
field then insert 'group': '__world__'.

This fixes a bug where people calling our API directly can create
annotations with no group field, then for example when the sidebar
searches for annotations with 'group': '__world__' it won't find these
annotations, but the Chrome extension badge (which doesn't put any group
filter in its searches) will find them.

This could be fixed client-side but it's better to make the annotations
in our search index consistent, than to try and always remember that
public annotations may either have group: '__world__' or no group at
all.

This means that it's no longer necessary for us to insert
group: '__world__' on the way out in render().

Annotations pre-dating this commit with no group can be migrated with:

    hypothesis annotool conf/production.ini prepare

Fixes #2589.
seanh added a commit that referenced this issue Oct 7, 2015
When creating or updating an annotation if the annotation has no 'group'
field then insert 'group': '__world__'.

This fixes a bug where people calling our API directly can create
annotations with no group field, then for example when the sidebar
searches for annotations with 'group': '__world__' it won't find these
annotations, but the Chrome extension badge (which doesn't put any group
filter in its searches) will find them.

This could be fixed client-side but it's better to make the annotations
in our search index consistent, than to try and always remember that
public annotations may either have group: '__world__' or no group at
all.

This means that it's no longer necessary for us to insert
group: '__world__' on the way out in render().

Annotations pre-dating this commit with no group can be migrated with:

    hypothesis annotool conf/production.ini prepare

Fixes #2589.
seanh added a commit that referenced this issue Oct 7, 2015
When creating or updating an annotation if the annotation has no 'group'
field then insert 'group': '__world__'.

This fixes a bug where people calling our API directly can create
annotations with no group field, then for example when the sidebar
searches for annotations with 'group': '__world__' it won't find these
annotations, but the Chrome extension badge (which doesn't put any group
filter in its searches) will find them.

This could be fixed client-side but it's better to make the annotations
in our search index consistent, than to try and always remember that
public annotations may either have group: '__world__' or no group at
all.

This means that it's no longer necessary for us to insert
group: '__world__' on the way out in render().

Annotations pre-dating this commit with no group can be migrated with:

    hypothesis annotool conf/production.ini prepare

Fixes #2589.
@nickstenning
Copy link
Contributor

I confirm this fixed on stage.

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

Successfully merging a pull request may close this issue.

3 participants