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: support serialization of Vaadin scoped beans #20394

Merged
merged 3 commits into from
Nov 7, 2024

Conversation

mcollovati
Copy link
Collaborator

Description

Makes sure that VaadinSession and UI thread locals are available during both serialization and deserialization, to allow other libraries to perform inspection and injection of Vaadin scoped beans.

Also refactors VaadinRouteScope to be independent from VaadinService when fetching RouteScopeOwner annotation for the bean, and replaces VaadinSession.unlock() calls with direct access to the lock instance to prevent unwanted push during bean lookup.

Fixes #19967
Part of vaadin/kubernetes-kit#140

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

Makes sure that VaadinSession and UI thread locals are available during
both serialization and deserialization, to allow other libraries to
perform inspection and injection of Vaadin scoped beans.

Also refactors VaadinRouteScope to be independent from VaadinService
when fetching RouteScopeOwner annotation for the bean, and replaces
VaadinSession.unlock() calls with direct access to the lock instance
to prevent unwanted push during bean lookup.

Fixes #19967
Part of vaadin/kubernetes-kit#140
mcollovati added a commit to vaadin/kubernetes-kit that referenced this pull request Nov 4, 2024
Vaadin scoped beans requires VaadinSession and UI thread locals to be set
in order to lookup beans.
This change tracks Vaadin scoped beans during serialization and makes sure
that required thread locals are set during deserialization.

The serialVersionUID added to TransientDescriptor should keep it compatible
with older version, preventing deserialization of previous data to fail.

Fixes #140
Requires vaadin/flow#20394
Copy link

github-actions bot commented Nov 4, 2024

Test Results

1 142 files  ± 0  1 142 suites  ±0   1h 24m 3s ⏱️ - 2m 11s
7 479 tests + 2  7 429 ✅ + 2  50 💤 ±0  0 ❌ ±0 
7 841 runs  +32  7 781 ✅ +31  60 💤 +1  0 ❌ ±0 

Results for commit 3679f56. ± Comparison against base commit 9378d46.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@mshabarov mshabarov left a comment

Choose a reason for hiding this comment

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

Do we need to double check CDI and Quarkus add-ons for the similar changes?

}
}

@Serial
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add @Serial to the same methods in VaadinSession class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, makes sense, even though it is not required

@@ -369,10 +355,52 @@ private NavigationListener getNavigationListener() {

}

private record ObjectWithOwner(Object object, RouteScopeOwner owner) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need implements Serializable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. The object is not stored anywhere. It is only used as a carrier of information for other logics.

@mcollovati
Copy link
Collaborator Author

Do we need to double check CDI and Quarkus add-ons for the similar changes?

Currently, custom logic for handling scoped beans during deserialization is implemented only in Kubernetes Kit and only for Spring.

Copy link

sonarcloud bot commented Nov 7, 2024

@mshabarov mshabarov merged commit a2fe8b6 into main Nov 7, 2024
26 checks passed
@mshabarov mshabarov deleted the issues/19967_serialization_vaadin_threadlocals branch November 7, 2024 11:05
vaadin-bot pushed a commit that referenced this pull request Nov 7, 2024
Makes sure that VaadinSession and UI thread locals are available during
both serialization and deserialization, to allow other libraries to
perform inspection and injection of Vaadin scoped beans.

Also refactors VaadinRouteScope to be independent from VaadinService
when fetching RouteScopeOwner annotation for the bean, and replaces
VaadinSession.unlock() calls with direct access to the lock instance
to prevent unwanted push during bean lookup.

Fixes #19967
Part of vaadin/kubernetes-kit#140

Co-authored-by: Mikhail Shabarov <[email protected]>
@vaadin-bot
Copy link
Collaborator

Hi @mcollovati and @mshabarov, when i performed cherry-pick to this commit to 24.4, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick a2fe8b6
error: could not apply a2fe8b6... fix: support serialization of Vaadin scoped beans (#20394)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

mcollovati added a commit that referenced this pull request Nov 7, 2024
Makes sure that VaadinSession and UI thread locals are available during
both serialization and deserialization, to allow other libraries to
perform inspection and injection of Vaadin scoped beans.

Also refactors VaadinRouteScope to be independent from VaadinService
when fetching RouteScopeOwner annotation for the bean, and replaces
VaadinSession.unlock() calls with direct access to the lock instance
to prevent unwanted push during bean lookup.

Fixes #19967
Part of vaadin/kubernetes-kit#140

Co-authored-by: Marco Collovati <[email protected]>
Co-authored-by: Mikhail Shabarov <[email protected]>
mcollovati added a commit that referenced this pull request Nov 7, 2024
Makes sure that VaadinSession and UI thread locals are available during
both serialization and deserialization, to allow other libraries to
perform inspection and injection of Vaadin scoped beans.

Also refactors VaadinRouteScope to be independent from VaadinService
when fetching RouteScopeOwner annotation for the bean, and replaces
VaadinSession.unlock() calls with direct access to the lock instance
to prevent unwanted push during bean lookup.

Fixes #19967
Part of vaadin/kubernetes-kit#140

Co-authored-by: Mikhail Shabarov <[email protected]>
mshabarov added a commit that referenced this pull request Nov 12, 2024
Makes sure that VaadinSession and UI thread locals are available during
both serialization and deserialization, to allow other libraries to
perform inspection and injection of Vaadin scoped beans.

Also refactors VaadinRouteScope to be independent from VaadinService
when fetching RouteScopeOwner annotation for the bean, and replaces
VaadinSession.unlock() calls with direct access to the lock instance
to prevent unwanted push during bean lookup.

Fixes #19967
Part of vaadin/kubernetes-kit#140

Co-authored-by: Mikhail Shabarov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Serialization of scoped beans fails due to the missing UI instance
3 participants