Update review list right after user posting/revoking by yiningwang11 · Pull Request #670 · eclipse/openvsx

yiningwang11 · 2023-02-02T21:50:31Z

fixes #613
The review list is not updated because it always gets loaded from the disk cache. Making the 'getReviews' always get data from the registry instead of from the cache fixes this issue.

amvanbaren · 2023-02-07T19:19:46Z

Thanks for your contribution @yiningwang11. It works as expected. I think it would be better to use the ETag header instead of fixed duration caching or no caching at all.

It's a combination of creating an ETag of the JSON response:

openvsx/server/src/main/java/org/eclipse/openvsx/web/ShallowEtagHeaderFilter.java

Lines 17 to 30 in 962947c

    
           protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException { 
        
               // limit the filter to /api/{namespace}/{extension}, /api/{namespace}/details 
        
               // and /api/{namespace}/{extension}/{version} endpoints 
        
               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("-"); 
        
               } 
        
               if(applyFilter && path.length == 4) { 
        
                   applyFilter = !(path[3].equals("review") || path[3].equals("reviews")); 
        
               } 
        
               return !applyFilter; 
        
           }

caching the response:

openvsx/server/src/main/java/org/eclipse/openvsx/LocalRegistryService.java

Lines 109 to 116 in 962947c

    
           @Override 
        
           @Cacheable(value = CACHE_EXTENSION_JSON, keyGenerator = GENERATOR_EXTENSION_JSON) 
        
           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); 
        
               json.downloads = getDownloads(extVersion.getExtension(), targetPlatform, extVersion.getVersion()); 
        
               return json; 
        
           }

and evicting the cache at the right time:

openvsx/server/src/main/java/org/eclipse/openvsx/LocalRegistryService.java

Line 707 in 962947c

cache.evictExtensionJsons(extension);

yiningwang11 · 2023-02-16T14:13:54Z

Thank you for your suggestion. I'll continue look into it :)

filiptronicek

Awesome, the reviews seem to be reflected instantly!

I think this issue is related -- the star icons don't seem to reflect the newly posted review.

yiningwang11 · 2023-03-06T15:00:54Z

Awesome, the reviews seem to be reflected instantly!

I think this issue is related -- the star icons don't seem to reflect the newly posted review.

I'll take a look on this. Thanks for your review!

filiptronicek

Nice! The extension ⭐ rating gets updated now as well. The only thing missing for me is the ⭐ rating update on the homepage. Could you please take a look? 🙏

(the highlighted extension I just reviewed)

filiptronicek · 2023-03-23T19:41:02Z

server/src/main/java/org/eclipse/openvsx/RegistryAPI.java


        return ResponseEntity.ok()
-                .cacheControl(CacheControl.maxAge(10, TimeUnit.MINUTES).cachePublic())
+                .cacheControl(CacheControl.noCache().cachePublic())


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

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)

amvanbaren · 2023-03-24T08:53:32Z

server/src/main/java/org/eclipse/openvsx/web/ShallowEtagHeaderFilter.java

    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


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

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.

@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.

amvanbaren · 2023-03-30T07:18:15Z

server/src/main/java/org/eclipse/openvsx/LocalRegistryService.java

    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);


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

amvanbaren · 2023-03-30T07:18:34Z

server/src/main/java/org/eclipse/openvsx/LocalRegistryService.java

        var json = toExtensionVersionJson(extVersion, targetPlatform, true, false);
+        var extension = repositories.findExtension(extensionName, namespace);
        json.downloads = getDownloads(extVersion.getExtension(), targetPlatform, extVersion.getVersion());
+        json.averageRating = extension.getAverageRating();


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

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.

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:

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

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 :)

Hi @yiningwang11, it looks like you merged openvsx:master on top of your updateReview branch. yiningwang11:master is still 70 commits behind eclipse: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?

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 :)

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.

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.

amvanbaren · 2023-03-30T07:20:44Z

server/src/main/java/org/eclipse/openvsx/LocalRegistryService.java

                .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();


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

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.

amvanbaren · 2023-03-30T07:35:03Z

server/src/main/java/org/eclipse/openvsx/web/ShallowEtagHeaderFilter.java

    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


@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.

amvanbaren · 2023-03-30T07:56:19Z

server/src/main/java/org/eclipse/openvsx/RegistryAPI.java

            try {
                return ResponseEntity.ok()
-                        .cacheControl(CacheControl.maxAge(10, TimeUnit.MINUTES).cachePublic())
+                        .cacheControl(CacheControl.noCache().cachePublic())


@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

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

amvanbaren · 2023-03-30T08:21:08Z

server/src/main/java/org/eclipse/openvsx/web/ShallowEtagHeaderFilter.java

+            applyFilter = path[2].contains("search");
        }
-        if(applyFilter && path.length == 4) {
-            applyFilter = !(path[3].equals("review") || path[3].equals("reviews"));


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

rebase branch "updateReview" onto branch "master"

* Access token dialog improvements * Set focus on the Copy button --------- Co-authored-by: amvanbaren <aart.vanbaren@eclipse-foundation.org>

* Search bar improvements * Remove 'Space' key in extension-list-header.tsx Change charCode 13 to key 'Enter' in namespace-input.tsx --------- Co-authored-by: amvanbaren <aart.vanbaren@eclipse-foundation.org>

Fixes eclipse#427 Change Namespace Evict caches Update search entries

Fixes eclipse#452 Added UpstreamProxyService Add url rewriting to UpstreamVSCodeService.java # Conflicts: # server/src/main/java/org/eclipse/openvsx/UpstreamRegistryService.java # server/src/main/java/org/eclipse/openvsx/adapter/UpstreamVSCodeService.java

Update extension publicId of already published extensions when a new extension has the same publicId

Use react-helmet to make <head> tags configurable through pageSettings for homepage, extension and namespace pages Simplify header tags Pass params instead of only name

bump webui version # Conflicts: # webui/package.json

Add possibility to delay running migrations, so that deployment can finish before migrations run

Added FixTargetPlatformMigration Remove extensionFile at end of a migration Don't run migrations in mirror mode

Improve social links UX

Use timestamp in namespace logo to bust browser cache

Currently copy-pasting some commands with smart quotes results in errors. Replace all the smart quotes with straight quotes for consistency. Signed-off-by: David Xia <david@davidxia.com>

server: move files to new namespace in background job webui: show confirmation dialog, instead of loading new namespace

Bumps [webpack](https://github.com/webpack/webpack) from 5.75.0 to 5.76.0. - [Release notes](https://github.com/webpack/webpack/releases) - [Commits](webpack/webpack@v5.75.0...v5.76.0) --- updated-dependencies: - dependency-name: webpack dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com>

Merge namespace members Return error if namespaces contain duplicate extensions

amvanbaren · 2023-06-20T19:11:26Z

@yiningwang11 I've opened PR #761. It contains your changes and clears the cache when a review is posted or deleted.

amvanbaren requested a review from filiptronicek February 7, 2023 19:19

filiptronicek reviewed Mar 3, 2023

View reviewed changes

yiningwang11 requested a review from filiptronicek March 17, 2023 13:26

filiptronicek reviewed Mar 17, 2023

View reviewed changes

yiningwang11 requested a review from filiptronicek March 23, 2023 17:18

filiptronicek reviewed Mar 23, 2023

View reviewed changes

amvanbaren reviewed Mar 24, 2023

View reviewed changes

yiningwang11 requested review from amvanbaren and filiptronicek and removed request for filiptronicek March 28, 2023 17:33

amvanbaren reviewed Mar 30, 2023

View reviewed changes

amvanbaren mentioned this pull request Mar 31, 2023

Cache /api/-/search results #542

Open

yiningwang11 and others added 15 commits April 1, 2023 20:54

fix issue 613

c559b7d

adding etag for getReviews

5525f1b

rebase branch "updateReview" onto branch "master"

unmodify CacheService.java

6fe5bd9

Update average rating according to review updates

f816b46

update ratings on homepage

bd5ae3d

Map.of doesnt allow null values

a6a9e61

IMDB rating

4b5e150

Access token dialog improvements (eclipse#661)

2c73daf

* Access token dialog improvements * Set focus on the Copy button --------- Co-authored-by: amvanbaren <aart.vanbaren@eclipse-foundation.org>

Search bar improvements (eclipse#662)

7bd25b6

* Search bar improvements * Remove 'Space' key in extension-list-header.tsx Change charCode 13 to key 'Enter' in namespace-input.tsx --------- Co-authored-by: amvanbaren <aart.vanbaren@eclipse-foundation.org>

[Admin] Add Ability to Change Namespace

30dbfef

Fixes eclipse#427 Change Namespace Evict caches Update search entries

Insert unique extension public id

0862c24

Update extension publicId of already published extensions when a new extension has the same publicId

website: improve information from search result

a9d453d

Use react-helmet to make <head> tags configurable through pageSettings for homepage, extension and namespace pages Simplify header tags Pass params instead of only name

export NamespaceDetailComponent

13594aa

bump webui version # Conflicts: # webui/package.json

Move mainHeadTags to main.tsx

8845246

amvanbaren and others added 26 commits April 1, 2023 21:01

use in clauses

03fb490

Delay running migrations

dd9735b

Add possibility to delay running migrations, so that deployment can finish before migrations run

Add namespace route to frontedRoutes

9141379

Create new StringBuilder

5680c22

Log query string too

03a5d03

Corrupted metadata extension in some extensions regarding targetPlatform

326f2e3

Added FixTargetPlatformMigration Remove extensionFile at end of a migration Don't run migrations in mirror mode

Set logoBytes before storing it

55810ad

Improve social links UX

Bump webui to 0.9.3

0e55bca

Get icon from storage

8794eb8

Use timestamp in namespace logo to bust browser cache

Delete temp file after use

af01986

Deprecate /api/-/query POST endpoint

17b966c

Move around some imports to rebuild postQuery Docker image

8f1319c

fix docs: only use straight quotes

5458a8e

Currently copy-pasting some commands with smart quotes results in errors. Replace all the smart quotes with straight quotes for consistency. Signed-off-by: David Xia <david@davidxia.com>

Renaming a namespace appears to leave incorrect URLs.

06234ce

server: move files to new namespace in background job webui: show confirmation dialog, instead of loading new namespace

Download count update partition ElasticSearch search entries update

fd51cc0

Remove headers logging

6d7d0a9

Bump webui to 0.9.5

0a6dc3c

Make user managed entity

61e1ec6

Fix 'Statement inside of curly braces should be on next line'

925711a

remove trailing space

29babd5

add set up guide for MacOS

7c2b79e

updated setup details

fbc1d8e

Small fixes

07db44b

Need to deal with collisions when renaming and merging namespaces

ec78245

Merge namespace members Return error if namespaces contain duplicate extensions

Merge ExtensionVersion before persisting FileResource

082d053

yiningwang11 force-pushed the updateReview branch from 668438a to 082d053 Compare April 2, 2023 01:02

Merge branch 'eclipse:master' into updateReview

a91d06e

amvanbaren closed this Jun 20, 2023

Conversation

yiningwang11 commented Feb 2, 2023

Uh oh!

amvanbaren commented Feb 7, 2023

Uh oh!

yiningwang11 commented Feb 16, 2023

Uh oh!

filiptronicek left a comment

Choose a reason for hiding this comment

Uh oh!

yiningwang11 commented Mar 6, 2023

Uh oh!

filiptronicek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yiningwang11 Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yiningwang11 Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amvanbaren Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amvanbaren commented Jun 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yiningwang11 Mar 24, 2023 •

edited

Loading

yiningwang11 Mar 30, 2023 •

edited

Loading

amvanbaren Mar 31, 2023 •

edited

Loading