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

Use the correct service name #2666

Closed
wants to merge 9 commits into from
Closed

Use the correct service name #2666

wants to merge 9 commits into from

Conversation

Legioth
Copy link
Member

@Legioth Legioth commented Aug 20, 2024

No description provided.

@CLAassistant
Copy link

CLAassistant commented Aug 20, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

codecov bot commented Aug 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.60%. Comparing base (52484b9) to head (0638cb8).
Report is 13 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2666   +/-   ##
=======================================
  Coverage   92.60%   92.60%           
=======================================
  Files          85       85           
  Lines        3164     3164           
  Branches      775      775           
=======================================
  Hits         2930     2930           
  Misses        183      183           
  Partials       51       51           
Flag Coverage Δ
unittests 92.60% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@taefi
Copy link
Contributor

taefi commented Aug 20, 2024

Funny that since we moved to servlet 5, no one complained about this, so either:

  • No one is deploying a war package on a servlet container, or
  • The com.vaadin.hilla.startup.EndpointsValidator is loaded by Flow automatically since it is implementing com.vaadin.flow.server.startup.ClassLoaderAwareServletContainerInitializer - not sure that's the reason.

Then, the question is: Do we actually need it?
Anyway, this change doesn't harm anyone and should have been applied sooner.

@Legioth
Copy link
Member Author

Legioth commented Aug 21, 2024

Should maybe also be cherry picked to 24.4.x?

@taefi
Copy link
Contributor

taefi commented Aug 21, 2024

Should maybe also be cherry picked to 24.4.x?

At least to 24.4.
2.5 is also using the jakarta namespace.

Copy link

sonarqubecloud bot commented Sep 2, 2024

Copy link
Contributor

@cromoteca cromoteca left a comment

Choose a reason for hiding this comment

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

This PR fixes the EndpointValidator registration in the servlet container, making it work for the first time. The purpose of this validator is to verify that Hilla endpoints are used in a Spring Boot application, and to throw an exception if endpoints exist and Spring Boot is not in the classpath.

The CSRF tests thus fail because they are not Spring Boot applications, but they do have an endpoint, which is SignalsHandler.

There are some possible solutions:

@cromoteca
Copy link
Contributor

Some of those changes could not propagate well to some of the cherry-pick target that have been selected for this PR.

@cromoteca
Copy link
Contributor

Also, given that it uses a ClassFinder from Flow, it seems to collect all classes annotated with BrowserCallable/Endpoint, but not all classes will be instantiated and used, so it seems a bit too strong as a check.

@taefi
Copy link
Contributor

taefi commented Jan 15, 2025

I'm in favor of logging a warning instead of throwing in EndpointValidator. A visible enough warning + documentation should be enough.

@cromoteca
Copy link
Contributor

I still see this as a sort of duplicate of #2938. While they serve different purposes, the check for the presence of Spring could be added at the same place instead of having to declare a dedicated listener.

@cromoteca
Copy link
Contributor

No longer needed after #3167

@cromoteca cromoteca closed this Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants