-
Notifications
You must be signed in to change notification settings - Fork 458
Fixed search page still returning stale data after invalidating a request #3888
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
Fixed search page still returning stale data after invalidating a request #3888
Conversation
…ad of waiting for non-stale response This was problematic for the places that used getFist operators. This is because they only emit data once, and the first value could be the old cached value
Also cleaned up test class
Checked this out real quick, but it looks like this doesn't immediately help with #3869. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks @alexandrevryghem . I tested this alongside DSpace/DSpace#10030 and #3892 . While I ran into small bugs with the other two PRs, I don't believe those were caused by this PR. I couldn't find any issues with this PR, as it appears to ensure the search results are returning properly.
That said, I'm going to wait to merge this until we get the bugs fixed in #3892 (especially), just to ensure there's no side effects being caused here.
That should also give @ybnd a chance to review this, if he has time. (per the above comment)
@alexandrevryghem and @ybnd : I was reminded about this PR today. Is this important to get into 8.1 / 7.6.3 (which are likely to come out this week)? I was waiting to merge this until @ybnd or someone else had a chance to give it another look. The door on 8.1 / 7.6.3 is closing quickly, and I'm not likely to merge much of anything else, unless it's a significant fix. I'm not able to reproduce "stale data" behavior (or haven't found a way to do so with this PR standalone), but I was able to verified this PR doesn't appear to cause side effects. |
@tdonohue: No one noticed this bug until now so for us it's fine to not merge this until DSpace/DSpace#10030 is merged, even if that's means that this PR will be delayed until 8.3 / 7.6.4 |
Hi @alexandrevryghem, |
…le-requests_contribute-main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Retested again today now that DSpace/DSpace#10030 is merged. Still looks good. Thanks again @alexandrevryghem
Successfully created backport PR for |
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin dspace-8_x
git worktree add -d .worktree/backport-3888-to-dspace-8_x origin/dspace-8_x
cd .worktree/backport-3888-to-dspace-8_x
git switch --create backport-3888-to-dspace-8_x
git cherry-pick -x edfaee6aab58bcb3b1f3d122d3b70276b0c0d8fa 58ff240c0525ce71c73046382f66cff458c970ad |
@alexandrevryghem : Could you manually backport this to 8.x? It looks like it ported automatically to 7.6.x, but the 8.x port failed. |
Description
Fixed problem with the
SearchService
methods that could return stale data. This was fixed in the same way asBaseDataService
was applied here to prevent outdated data from being emitted.Instructions for Reviewers
List of changes in this PR:
skipWhile
method to both thesearch
&getFacetValuesFor
to skip stale data and to ignore non stale data whenuseCachedVersionIfAvailable
istrue
SearchService
test classGuidance for how to test or review your PR:
Checklist
main
branch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lint
npm run check-circ-deps
)package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.