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

Require transitively io.vertx.auth.common module #2714

Closed

Conversation

tsegismont
Copy link
Contributor

This module is required by Vert.x Web applications even when no Vert.x Auth handler is used.

Indeed, Vert.x Web uses the SecurityAudit class in several places.

Instead of making all JPMS users of Vert.x Web require that module explicitly, we should make it a transitive requirement.

This module is required by Vert.x Web applications even when no Vert.x Auth handler is used.

Indeed, Vert.x Web uses the SecurityAudit class in several places.

Instead of making all JPMS users of Vert.x Web require that module explicitly, we should make it a transitive requirement.

Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
@tsegismont tsegismont added this to the 5.0.0 milestone Feb 4, 2025
@tsegismont tsegismont requested a review from vietj February 4, 2025 13:30
@vietj
Copy link
Contributor

vietj commented Feb 4, 2025

the issue with this is that we might have to add many transitive requires for other modules like vertx-core and I would prefer we first define clearly how we operate if we go this way

@tsegismont
Copy link
Contributor Author

Thinking about an RFC?

@vietj
Copy link
Contributor

vietj commented Feb 4, 2025

thinking about understanding first what are the implications and best practices:

  • if we add an extra transitive and remove it after (because it was actually not needed), is it considered as a breaking change ?
  • how to keep a minimal set of transitive, e.g. if vertx-web depends on vertx-web-common and vertx-web-common requires transitively vertx-core, then vertx-web does not need to declare it transitively ?

@tsegismont
Copy link
Contributor Author

These are good questions, indeed. I'll add that to an RFC, unless you prefer some other material

@tsegismont
Copy link
Contributor Author

In fact, the problem is not that the Vert.x Web module should transitively require the Vert.x Auth Common module. The SecurityAudit class is not necessary for users, it's required by Vert.x Web only.

So we should change the declaration, but only to a simple requirement, not transitive. I'll send another PR

@tsegismont tsegismont closed this Feb 7, 2025
@tsegismont tsegismont deleted the requires-transitive-auth-common branch February 7, 2025 09:50
@tsegismont tsegismont removed this from the 5.0.0 milestone Feb 7, 2025
@tsegismont
Copy link
Contributor Author

See #2716

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants