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

Fix message headers that weren't set in the http response headers. #2564

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luankevinferreira
Copy link

@luankevinferreira luankevinferreira commented Feb 1, 2024

Motivation:

Messages sent over the event bus can also contain headers. This can be specified by providing a DeliveryOptions when sending or publishing.

The following changes aim to fix the following problems in this flow:

  • DeliveryOptions headers are not being set in the HTTP responses for message replies;
  • Multi-value headers (like Set-Cookie) are not being set in the HTTP responses because it's not retrieving all the key values from the MultiMap;

@luankevinferreira luankevinferreira force-pushed the fix-message-headers-from-delivery-options-to-http-response branch from d75a6bb to 3c40c94 Compare February 1, 2024 16:39
Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

Thanks for reporting this @luankevinferreira

Indeed, multi-value headers should be supported, can you add a test for it?

Nevertheless, I'm not convinced response message headers should be copied to the http response, because they can be defined in the ServiceResponse object.

@luankevinferreira
Copy link
Author

Nevertheless, I'm not convinced response message headers should be copied to the http response, because they can be defined in the ServiceResponse object.

The problem with ServiceResponse is that JSON objects don't allow duplicated keys, which adds the limitation of having multiple headers like Set-Cookie, and since JSON is a convention to be used along with event bus like mentioned in the documentation, it would be required to be set by another way like the DeliveryOptions

See: https://vertx.io/docs/vertx-core/java/#_types_of_messages

@luankevinferreira luankevinferreira force-pushed the fix-message-headers-from-delivery-options-to-http-response branch from 3c40c94 to abc7721 Compare February 8, 2024 15:01
Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

Thank you for the updates

@tsegismont
Copy link
Contributor

The problem with ServiceResponse is that JSON objects don't allow duplicated keys

That's right, thank you for spotting this. Then I think that we should store headers in the service response in the form of an array of json objects like this:

{
  "headers": [
    { "header1": "value1"},
    { "header1": "value2"},
    { "header2": "value3"},
  ]
}

And for backward compatibility, perhaps we can have a sysprop that switches between the old and the new format

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