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

WebSockets Next: make it possible to store user data in a connection #43787

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Oct 9, 2024

@mkouba mkouba force-pushed the issue-43772 branch 2 times, most recently from 54d9914 to 6b36a91 Compare October 10, 2024 07:19
@mkouba mkouba marked this pull request as ready for review October 10, 2024 07:20

This comment has been minimized.

This comment has been minimized.

Copy link

github-actions bot commented Oct 10, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

/**
* @param <TYPE> The type this key is used for.
*/
record TypedKey<TYPE>(String value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't really need the string key. The object identity of the key is itself good enough.

This

static class Key<T> {
}

is all you need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. This way you would need to use the object reference as a key. Always. That's not what we want/need. A string-based key is IMO more practical.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, you're risking collisions, so the string keys will have to be namespaced... Not a fan. But your choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for user data. I don't expect collisions there. Anyway, we risk collisions pretty much everywhere when it comes to MP Config and application properties 🤷.

*/
record TypedKey<TYPE>(String value) {

public static TypedKey<Integer> forInt(String val) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be an int here?

Suggested change
public static TypedKey<Integer> forInt(String val) {
public static TypedKey<Integer> forInt(int val) {

Copy link
Contributor

Choose a reason for hiding this comment

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

and then return new TypedKey<>(String.valueOf(val));?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The value is actually the key. I should rename the param.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In other words, this factory method is used to create a typed key with the specified string value. I.e. you expect the value to be integer, but the key is still string-based.

Copy link
Contributor

@gastaldi gastaldi Oct 10, 2024

Choose a reason for hiding this comment

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

Can we hide this implementation detail by providing extra put/get/remove methods in the UserData?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually not an implementation detail. We want the users to use the typed key in order to use generics to verify the type of the value in put() and to cast the value automatically when obtaining the value with get(). We could add getString() and putString() etc. but I don't think it would make the API more clear or useful. Also we would have to keep the generic way so that it could be used for "custom" types, similar to https://github.com/quarkusio/quarkus/pull/43787/files#diff-a63047e6c7bf732258f9858dcb9cb5303fa5ebe9756eb27cc970a4130190bdc3R62.

Copy link

quarkus-bot bot commented Oct 10, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit a3d3a32.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Oct 10, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit a3d3a32.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@cescoffier cescoffier merged commit 8fa2763 into quarkusio:main Oct 11, 2024
23 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Oct 11, 2024
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Oct 11, 2024
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.

websocket-next api - missing user properties map
4 participants