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

WebSocket automatic reflection registration #31972

Closed
amoscatelli opened this issue Mar 20, 2023 · 9 comments · Fixed by #32018
Closed

WebSocket automatic reflection registration #31972

amoscatelli opened this issue Mar 20, 2023 · 9 comments · Fixed by #32018
Labels
Milestone

Comments

@amoscatelli
Copy link
Contributor

Description

Hi guys !
I think that quarkus should automatically register for reflections classes to serialize or deserialize in websockets.
This should be similar to what it's done for REST endpoints.

What do you think ?

Implementation ideas

I think this could be done inside a BuildStep, scanning jandex index, looking for implementations of jakarta.websocket.Decoder or jakarta.websocket.Encoder and producing ReflectiveHierarchyBuildItem accordingly.

@amoscatelli amoscatelli added the kind/enhancement New feature or request label Mar 20, 2023
@amoscatelli amoscatelli changed the title WebSocket Automatic Reflection WebSocket Automatic Reflection registration Mar 20, 2023
@amoscatelli amoscatelli changed the title WebSocket Automatic Reflection registration WebSocket automatic reflection registration Mar 20, 2023
@amoscatelli
Copy link
Contributor Author

@geoand
Copy link
Contributor

geoand commented Mar 20, 2023

Hi,

Do you have some sample application that fails in native mode as is?

@amoscatelli
Copy link
Contributor Author

amoscatelli commented Mar 20, 2023

@geoand Ummmn it's quite hard to extract a sample application but I just created a web socket like this :

@ServerEndpoint(
        value = "/chat",
        encoders = ChatMessageEncoder.class,
        decoders = ChatMessageDecoder.class
)
public class ChatSocket {

    @OnMessage
    public void onMessage(ChatMessageDTO message) {
        chatController.publish(
                this.mapper.from(
                        message
                )
        );
    }
    
}
public class ChatMessageDTO {
    
    private String id;
    
    private String from;

    private String content;

    public String getId() {
        return id;
    }

    public void setId(String id) {
        this.id = id;
    }
    
    public String getFrom() {
        return from;
    }

    public void setFrom(String from) {
        this.from = from;
    }
    
    public String getContent() {
        return content;
    }

    public void setContent(String content) {
        this.content = content;
    }
    
}

And the only way I have to make it work is to have this reflection-config.json :

[
    {
        "name": "com.techedge.dto.ChatMessageDTO",
        "allDeclaredConstructors": true,
        "allPublicConstructors": true,
        "allDeclaredMethods": true,
        "allPublicMethods": true,
        "allDeclaredFields": true,
        "allPublicFields": true
    }
] 

I believe this should work out of the box.
Without reflection-config.json I can see errors about missing constructor while deploying on my kubernetes in native mode.

Anyway you seem surprised, is this supposed to work already ?
That's strange since I can't see anything about registering types in the websocket processor ...

@geoand
Copy link
Contributor

geoand commented Mar 21, 2023

Okay, then yeah, I think it makes sense.

Feel free to contribute this fix :)

@amoscatelli
Copy link
Contributor Author

I'll work on it in the next days !
I believe it'll be pretty simple

@geoand
Copy link
Contributor

geoand commented Mar 21, 2023

Thanks. yeah it should be straightfoward.

@amoscatelli
Copy link
Contributor Author

amoscatelli commented Mar 21, 2023

@geoand here you are !
#32018

@amoscatelli
Copy link
Contributor Author

This works correctly now in CR1 !

@geoand
Copy link
Contributor

geoand commented Mar 31, 2023

🎉

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

Successfully merging a pull request may close this issue.

2 participants