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

Ws{{Action}}Context classes do not have a pretty getUpgradeContext() method. #2216

Open
LoonyRules opened this issue Apr 7, 2024 · 3 comments

Comments

@LoonyRules
Copy link
Contributor

Describe your issue
The WsConnectContext, WsCloseContext, WsErrorContext, WsMessageContext, and WsBinaryMessageContext classes do not have a pretty way to retrieve the Context that was used during the Upgrade request via the Java Api.

The current usage is:

public void onWsConnect(final @NotNull WsConnectContext ctx) {
  final Context upgradeContext = ctx.getUpgradeCtx$javalin();
  
  // ... do something with the upgradeContext, like `upgradeContext.path()`.
}

but I'd prefer for the usage to be:

public void onWsConnect(final @NotNull WsConnectContext ctx) {
  final Context upgradeContext = ctx.getUpgradeCtx();
  
  // ... do something with the upgradeContext, like `upgradeContext.path()`.
}

So this change is purely a aesthetic request, the underlying functionality should not change.

@dzikoysk
Copy link
Member

dzikoysk commented Apr 7, 2024

The current upgradeContext property is marked as internal - it means that it shouldn't be visible for you in the first place, so as a root cause I'd mark this as a bug, because it's missing the @JvmSynthetic annotation. If we'd like to open that part of the API, it might be good idea to investigate the pros/cons of doing this, because maybe it'd make more sense to expose only some particular functions like e.g. mentioned path() - that's the approach we already use for half of the methods in this class.

@LoonyRules
Copy link
Contributor Author

Ah yeah @tipsy did mention this in Discord but I just forgot to mention that here. Feel free to adjust this to be a Bug then, with another Issue span off discussing the lack of API method for Ws related contexts if that'd how you'd like to manage this? :)

@tipsy
Copy link
Member

tipsy commented Apr 8, 2024

The current upgradeContext property is marked as internal - it means that it shouldn't be visible for you in the first place, so as a root cause I'd mark this as a bug,

I agree that this is a bug, but unfortunately we can't add this annotation now (the method has been public for years). So I chose feature request rather than bug.

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

No branches or pull requests

3 participants