Skip to content

Update review list right after user posting/revoking #670

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.

Sign up for GitHub

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

Closed
yiningwang11 wants to merge 46 commits into from
Closed

Update review list right after user posting/revoking #670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
c559b7d
fix issue 613
yiningwang11 Feb 2, 2023
5525f1b
adding etag for getReviews
yiningwang11 Feb 28, 2023
6fe5bd9
unmodify CacheService.java
yiningwang11 Feb 28, 2023
f816b46
Update average rating according to review updates
yiningwang11 Mar 14, 2023
bd5ae3d
update ratings on homepage
yiningwang11 Mar 23, 2023
a6a9e61
Map.of doesnt allow null values
jeanp413 Jan 31, 2023
4b5e150
IMDB rating
Feb 2, 2023
2c73daf
Access token dialog improvements (#661)
filiptronicek Feb 13, 2023
7bd25b6
Search bar improvements (#662)
filiptronicek Feb 14, 2023
30dbfef
[Admin] Add Ability to Change Namespace
Jan 5, 2023
4544beb
Add a way to proxy upstream requests instead of external links
Feb 20, 2023
0862c24
Insert unique extension public id
Feb 22, 2023
a9d453d
website: improve information from search result
Feb 21, 2023
13594aa
export NamespaceDetailComponent
Feb 23, 2023
8845246
Move mainHeadTags to main.tsx
Feb 23, 2023
24336c7
Update test extensions
filiptronicek Feb 23, 2023
7ff8e30
Merge entity before calling remove
Feb 24, 2023
2e7e8e1
Read input stream before try to get VSIX manifest from zip file
Feb 25, 2023
7a16ecb
Use explicit joins to improve query performance
Feb 28, 2023
03fb490
use in clauses
Feb 28, 2023
dd9735b
Delay running migrations
Mar 1, 2023
9141379
Add namespace route to frontedRoutes
Mar 6, 2023
5680c22
Create new StringBuilder
Mar 9, 2023
03a5d03
Log query string too
Mar 9, 2023
326f2e3
Corrupted metadata extension in some extensions regarding targetPlatform
Mar 1, 2023
55810ad
Set logoBytes before storing it
Mar 9, 2023
0e55bca
Bump webui to 0.9.3
Mar 9, 2023
8794eb8
Get icon from storage
Mar 10, 2023
af01986
Delete temp file after use
Mar 15, 2023
17b966c
Deprecate `/api/-/query` POST endpoint
Mar 15, 2023
8f1319c
Move around some imports to rebuild postQuery Docker image
Mar 16, 2023
5458a8e
fix docs: only use straight quotes
davidxia Mar 18, 2023
06234ce
Renaming a namespace appears to leave incorrect URLs.
Mar 15, 2023
fd51cc0
Download count update partition ElasticSearch search entries update
Mar 21, 2023
e590b64
Bump webpack from 5.75.0 to 5.76.0 in /webui
dependabot[bot] Mar 15, 2023
6d7d0a9
Remove headers logging
Mar 22, 2023
0a6dc3c
Bump webui to 0.9.5
Mar 22, 2023
61e1ec6
Make user managed entity
Mar 22, 2023
925711a
Fix 'Statement inside of curly braces should be on next line'
Mar 22, 2023
29babd5
remove trailing space
Mar 22, 2023
7c2b79e
add set up guide for MacOS
yiningwang11 Feb 7, 2023
fbc1d8e
updated setup details
yiningwang11 Feb 23, 2023
07db44b
Small fixes
filiptronicek Mar 27, 2023
ec78245
Need to deal with collisions when renaming and merging namespaces
Mar 23, 2023
082d053
Merge ExtensionVersion before persisting FileResource
Mar 31, 2023
a91d06e
Merge branch 'eclipse:master' into updateReview
yiningwang11 Apr 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions server/src/main/java/org/eclipse/openvsx/LocalRegistryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,9 @@ public ExtensionJson getExtension(String namespace, String extensionName, String
public ExtensionJson getExtension(String namespace, String extensionName, String targetPlatform, String version) {
var extVersion = findExtensionVersion(namespace, extensionName, targetPlatform, version);
var json = toExtensionVersionJson(extVersion, targetPlatform, true, false);
var extension = repositories.findExtension(extensionName, namespace);
Copy link
Contributor

@amvanbaren amvanbaren Mar 30, 2023

Choose a reason for hiding this comment

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

Please remove this statement, because you don't have to set averageRating here.

json.downloads = getDownloads(extVersion.getExtension(), targetPlatform, extVersion.getVersion());
json.averageRating = extension.getAverageRating();
Copy link
Contributor

@amvanbaren amvanbaren Mar 30, 2023

Choose a reason for hiding this comment

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

Please remove this statement, because averageRating is already set in ExtensionVersion.toExtensionJson.

Copy link
Contributor Author

@yiningwang11 yiningwang11 Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hello @amvanbaren , thanks for your comment!
The reason that I set averageRating here is because I found out in ExtensionVersion.toExtensionJson, the averageRating is not updated immediately according to deleting/posting new reviews with new ratings of an extension. However, the extension.averageRating is being updated correctly in time. That's why I put an extra statement here to update the averageRating.

Copy link
Contributor

@amvanbaren amvanbaren Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I see what you mean. I went through the code to find out how this could happen. The VersionService caches the latest ExtensionVersion. When a review is posted or deleted your fork doesn't evict the latest version cache, which causes findExtensionVersion to return a cached instance of the lastest ExtensionVersion with an old Extension instance (averageRating not updated).

This has been fixed in master. Can you sync your master branch and rebase the updateReview branch? Then the extra database call shouldn't be necessary anymore.

You can sync the master branch by clicking Sync fork in the right corner of your Github repo:
Schermafbeelding 2023-03-31 om 11 46 01

After syncing your fork you can rebase by opening a terminal in your openvsx directory and following these steps:

git checkout master
git pull
git checkout updateReview
git rebase master
<FIX CONFLICTS & COMMIT>
git rebase --continue
git push -f origin updateReview

Copy link
Contributor Author

@yiningwang11 yiningwang11 Apr 2, 2023

Choose a reason for hiding this comment

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

Hello @amvanbaren , thanks for your suggestion!
I've followed these commands but seems like git push -f origin updateReview has caused one of the checks to be failing. It looks like one of the authors of the commits is not covered by the ECA. I'm not sure if that's gonna affect anything, or what should I do to fix this?
Thank you again for your help :)

Copy link
Contributor

@amvanbaren amvanbaren Apr 3, 2023

Choose a reason for hiding this comment

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

Hi @yiningwang11, it looks like you merged openvsx:master on top of your updateReview branch. yiningwang11:master is still 70 commits behind eclipse:master.
yiningwang11-master

A rebase changes the base of the branch, but the changes you've made stay on top. This article explains it in further detail: https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase. The example under the Usage heading describes what we try to do here.

Are the top 5 commits for this feature or were there more commits?
yiningwang11-commits

Copy link
Contributor Author

@yiningwang11 yiningwang11 Apr 11, 2023

Choose a reason for hiding this comment

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

Hello @amvanbaren , thanks for your comment! I went over the article you recommended. Is this because I should have sync my yiningwang11:master branch and then rebase the updateReview feature branch, instead of merge the openvsx:master on top of my updateReview feature branch?
Is there anything I can do to make things right at the moment?

The top 5 commits in the screenshot you shared were for this feature, there aren't any more commits yet. (I'm still working on adding the @Cacheable annotation on LocalRegistryService.getReviews now)

Thank you :)

Copy link
Contributor

@amvanbaren amvanbaren Apr 17, 2023

Choose a reason for hiding this comment

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

Is this because I should have sync my yiningwang11:master branch and then rebase the updateReview feature branch, instead of merge the openvsx:master on top of my updateReview feature branch?

Yes, it is so that this feature is current with master. This ensures that you don't run into issues that already have been fixed in master and that there are no conflicts when this branch is merged into master.

Is there anything I can do to make things right at the moment?

Yes, you can use git rebase to drop the unwanted commits.
First, do git stash to store your current changes.
Then set VS Code as your default git editor. You can skip this step if you're familiar with vim.

git config core.editor "code --wait"

Next, do an interactive rebase on your updateReview branch. I see this PR has 46 commits. If you have more commits locally then increase the number in the below command, e.g. you've added 2 more commits, then the number becomes 48.

git rebase -i HEAD~46

You get a list of commits. You need to keep the first 5 commits and if you've added extra commits since this PR you want to keep those too. All the other commits in between you want to drop.
rebase-drop

When you're 100% sure (you can't undo this operation) which commits to keep and which to drop, close the editor. The rebase is applied and the commits from the merge with master should be gone.

Force push updateReview

git push -f origin updateReview

If you used git stash, you can use git stash apply to get your local working changes back.

return json;
}

Expand Down Expand Up @@ -772,6 +774,7 @@ private List<SearchEntryJson> toSearchEntries(SearchHits<ExtensionSearch> search
.map(e -> {
var entry = e.getValue().toSearchEntryJson();
entry.url = createApiUrl(serverUrl, "api", entry.namespace, entry.name);
entry.averageRating = repositories.findExtension(entry.name, entry.namespace).getAverageRating();
Copy link
Contributor

@amvanbaren amvanbaren Mar 30, 2023

Choose a reason for hiding this comment

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

Please remove this statement, because averageRating is already set in ExtensionVersion.toSearchEntryJson.

Copy link
Contributor Author

@yiningwang11 yiningwang11 Mar 30, 2023

Choose a reason for hiding this comment

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

I added this statement due to the same reason. I found ExtensionVersion.toSearchEntryJson is not updating averageRating attribute according to deleting/posting reviews with new ratings.

return new AbstractMap.SimpleEntry<>(e.getKey(), entry);
})
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/eclipse/openvsx/RegistryAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ public ResponseEntity<ReviewListJson> getReviews(
for (var registry : getRegistries()) {
try {
return ResponseEntity.ok()
.cacheControl(CacheControl.maxAge(10, TimeUnit.MINUTES).cachePublic())
.cacheControl(CacheControl.noCache().cachePublic())
Copy link
Contributor

@amvanbaren amvanbaren Mar 30, 2023

Choose a reason for hiding this comment

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

@yiningwang11 Can you also make LocalRegistryService.getReviews @Cacheable? Please make sure to evict entries from the cache when a review is added (LocalRegistryService.postReview) or deleted (LocalRegistryService.deleteReview) and when UserData changes in UserService.updateExistingUser

Copy link
Contributor Author

@yiningwang11 yiningwang11 Mar 30, 2023

Choose a reason for hiding this comment

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

@amvanbaren I'll take a look into this. Thank you!

.body(registry.getReviews(namespace, extension));
} catch (NotFoundException exc) {
// Try the next registry
Expand Down Expand Up @@ -745,7 +745,7 @@ public ResponseEntity<SearchResultJson> search(
}

return ResponseEntity.ok()
.cacheControl(CacheControl.maxAge(10, TimeUnit.MINUTES).cachePublic())
.cacheControl(CacheControl.noCache().cachePublic())
Copy link
Contributor

@filiptronicek filiptronicek Mar 23, 2023

Choose a reason for hiding this comment

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

I wonder if there is a better way of replacing the cache instead of disabling it altogether? cc @amvanbaren

Otherwise I'm good with the changes

Copy link
Contributor

@amvanbaren amvanbaren Mar 24, 2023

Choose a reason for hiding this comment

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

I wonder if there is a better way of replacing the cache instead of disabling it altogether?

Yes, a combination of server caching and generating an ETag for the response. If the request contains the If-None-Match header and the value matches the ETag value, then no content (204) is returned because the client already has the most up to date response. #670 (comment)

.body(result);
}

Expand Down
1 change: 1 addition & 0 deletions server/src/main/java/org/eclipse/openvsx/cache/CacheService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.eclipse.openvsx.repositories.RepositoryService;
import org.eclipse.openvsx.util.TargetPlatform;
import org.eclipse.openvsx.util.VersionAlias;
import org.eclipse.openvsx.util.UrlUtil;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.cache.CacheManager;
import org.springframework.stereotype.Component;
Expand Down
12 changes: 4 additions & 8 deletions server/src/main/java/org/eclipse/openvsx/web/ShallowEtagHeaderFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,12 @@ public class ShallowEtagHeaderFilter extends org.springframework.web.filter.Shal

protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException {
// limit the filter to /api/{namespace}/{extension}, /api/{namespace}/details
// and /api/{namespace}/{extension}/{version} endpoints
// and /api/{namespace}/{extension}/{version}, and /api/-/search endpoints
Copy link
Contributor

@amvanbaren amvanbaren Mar 24, 2023

Choose a reason for hiding this comment

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

@yiningwang11 Why generate an ETag for the /api/-/search endpoint?

Copy link
Contributor Author

@yiningwang11 yiningwang11 Mar 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, @amvanbaren thanks for the review!
I added this to generate an ETag for the response because I have disabled the cache for the search endpoint that returns the homepage. I thought with an ETag coming with the response header, even though the cache is disabled, no content would still be returned if the client has the up-to-date response.

Copy link
Contributor

@amvanbaren amvanbaren Mar 30, 2023

Choose a reason for hiding this comment

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

@yiningwang11 Ok, that's true. The downside is that the search is fully processed. Only at the end the ShallowEtagHeaderFilter compares the If-None-Match request header with the hash of the JSON body and discards the body if the two Etags match.

On the other hand #542 caches search results on the server side, so this approach will work. I'll test #542 and merge it in.

var path = request.getRequestURI().substring(1).split("/");
var applyFilter = path.length == 3 || path.length == 4;
if(applyFilter) {
applyFilter = path[0].equals("api") && !path[1].equals("-");
var applyFilter = (path.length == 3 || path.length == 4) && path[0].equals("api");
if(applyFilter && path[1].equals("-")) {
applyFilter = path[2].contains("search");
}
if(applyFilter && path.length == 4) {
applyFilter = !(path[3].equals("review") || path[3].equals("reviews"));
Copy link
Contributor

@amvanbaren amvanbaren Mar 30, 2023

Choose a reason for hiding this comment

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

Please keep applyFilter = !(path[3].equals("review");, because I think we don't want to use ETags when a review is added or deleted.

}

return !applyFilter;
}
}
4 changes: 2 additions & 2 deletions webui/src/pages/extension-detail/extension-review-dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,9 @@ class ExtensionReviewDialogComponent extends React.Component<ExtensionReviewDial
return null;
}
return <React.Fragment>
<Button variant='contained' color='secondary' onClick={this.handleOpenButton}>
{!this.state.posted && (<Button variant='contained' color='secondary' onClick={this.handleOpenButton}>
Write a Review
</Button>
</Button>)}
<Dialog open={this.state.open} onClose={this.handleCancel}>
<DialogTitle>{this.props.extension.displayName || this.props.extension.name} Review</DialogTitle>
<DialogContent>
Expand Down