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

fix: Fix regression when sync id check is disabled #17238

Merged
merged 4 commits into from Jul 20, 2023
Merged

Conversation

TatuLund
Copy link
Contributor

Fixes: #17237

@sonatype-lift
Copy link

sonatype-lift bot commented Jul 19, 2023

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

@mcollovati mcollovati added the Contribution PRs coming from the community or external to the team label Jul 19, 2023
@mcollovati mcollovati added this to Iteration Reviews in OLD Vaadin Flow ongoing work (Vaadin 10+) via automation Jul 19, 2023
@github-actions
Copy link

github-actions bot commented Jul 19, 2023

Test Results

   998 files  ±0     998 suites  ±0   1h 0m 53s ⏱️ + 1m 4s
6 350 tests +1  6 309 ✔️ +1  41 💤 ±0  0 ±0 
6 609 runs  +6  6 561 ✔️ +6  48 💤 ±0  0 ±0 

Results for commit 5324be8. ± Comparison against base commit 9e0ef2f.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@mcollovati mcollovati left a comment

Choose a reason for hiding this comment

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

@TatuLund thank you very much for spotting the bug and providing the fix.
The change looks good to me, but I would like to add a test for this case

Could use please add a test to LongPollingCacheFilterTest class?
Something like

    @Test
    public void filter_syncIdCheckDisabled_continueWithCurrentMessage() {
        // a value of -1 means that syncId check is disabled
        setSeenServerSyncIdHeader(-1);
        setTransport(AtmosphereResource.TRANSPORT.LONG_POLLING);
        simulatePushConnection();
        BroadcastAction action = filter.filter("broadcasterId", resource,
                originalMessage, message);
        Assert.assertEquals(ACTION.CONTINUE, action.action());
        Assert.assertSame(
                "Message should not be altered by filter if syncId check is disabled",
                message, action.message());
        verifyMessageIsNotCached();
    }

@TatuLund
Copy link
Contributor Author

TatuLund commented Jul 19, 2023

Could use please add a test to LongPollingCacheFilterTest class?

I was already working on that, now there

@sonarcloud
Copy link

sonarcloud bot commented Jul 19, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mcollovati mcollovati merged commit 862d34d into main Jul 20, 2023
27 checks passed
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Iteration Reviews to Done - pending release Jul 20, 2023
@mcollovati mcollovati deleted the TatuLund-patch-1 branch July 20, 2023 04:49
vaadin-bot pushed a commit that referenced this pull request Jul 20, 2023
When syncId check is disabled, long-polling push cache filter always cached all messages.
This change fixes the regression, by preventing message cache when syncId check is disabled.
vaadin-bot pushed a commit that referenced this pull request Jul 20, 2023
When syncId check is disabled, long-polling push cache filter always cached all messages.
This change fixes the regression, by preventing message cache when syncId check is disabled.
vaadin-bot pushed a commit that referenced this pull request Jul 20, 2023
When syncId check is disabled, long-polling push cache filter always cached all messages.
This change fixes the regression, by preventing message cache when syncId check is disabled.
vaadin-bot pushed a commit that referenced this pull request Jul 20, 2023
When syncId check is disabled, long-polling push cache filter always cached all messages.
This change fixes the regression, by preventing message cache when syncId check is disabled.
vaadin-bot pushed a commit that referenced this pull request Jul 20, 2023
When syncId check is disabled, long-polling push cache filter always cached all messages.
This change fixes the regression, by preventing message cache when syncId check is disabled.
vaadin-bot added a commit that referenced this pull request Jul 20, 2023
When syncId check is disabled, long-polling push cache filter always cached all messages.
This change fixes the regression, by preventing message cache when syncId check is disabled.

Co-authored-by: Tatu Lund <tatu@vaadin.com>
vaadin-bot added a commit that referenced this pull request Jul 20, 2023
When syncId check is disabled, long-polling push cache filter always cached all messages.
This change fixes the regression, by preventing message cache when syncId check is disabled.

Co-authored-by: Tatu Lund <tatu@vaadin.com>
vaadin-bot added a commit that referenced this pull request Jul 20, 2023
When syncId check is disabled, long-polling push cache filter always cached all messages.
This change fixes the regression, by preventing message cache when syncId check is disabled.

Co-authored-by: Tatu Lund <tatu@vaadin.com>
vaadin-bot added a commit that referenced this pull request Jul 20, 2023
When syncId check is disabled, long-polling push cache filter always cached all messages.
This change fixes the regression, by preventing message cache when syncId check is disabled.

Co-authored-by: Tatu Lund <tatu@vaadin.com>
vaadin-bot added a commit that referenced this pull request Jul 20, 2023
When syncId check is disabled, long-polling push cache filter always cached all messages.
This change fixes the regression, by preventing message cache when syncId check is disabled.

Co-authored-by: Tatu Lund <tatu@vaadin.com>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.2.0.alpha5 and is also targeting the upcoming stable 24.2.0 version.

mshabarov pushed a commit that referenced this pull request Aug 7, 2023
When syncId check is disabled, long-polling push cache filter always cached all messages.
This change fixes the regression, by preventing message cache when syncId check is disabled.

Co-authored-by: Tatu Lund <tatu@vaadin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Development

Successfully merging this pull request may close these issues.

Disabling sync id checking does not work
3 participants