Skip to content

CAMEL-14074 -> Custom host header implementation. - #3403

Merged
omarsmak merged 2 commits into
apache:masterfrom
djoleB:CAMEL-14074
Dec 17, 2019
Merged

CAMEL-14074 -> Custom host header implementation.#3403
omarsmak merged 2 commits into
apache:masterfrom
djoleB:CAMEL-14074

Conversation

@djoleB

@djoleB djoleB commented Dec 11, 2019

Copy link
Copy Markdown
Contributor

note: this feature will work only with java >=12. Because that's when they enabled host overriding.

@djoleB djoleB changed the title CAMEL-14074 - WIP -> Custom host header implementation. CAMEL-14074 -> Custom host header implementation. Dec 11, 2019

@omarsmak omarsmak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @djoleB for the PR! I left few nit comments that can help further.

import org.apache.camel.support.MessageHelper;
import org.apache.camel.support.ObjectHelper;
import org.apache.camel.support.SynchronizationAdapter;
import org.apache.camel.support.*;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To import all packages is not preferable, did you run camel build with source check in order to pick up these issues?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, it didn't complain. This is how my IDE is set, i can revert it if it's a problem.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pretty weird, it should complain. Can you please revert it?

}

@Test
public void testHttpProducer_GivenCustomHostHeaderQuery_SetCustomHost() throws Exception {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the names of the test have to contain _ ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Of course it doesn't, i find it more readable this way.

@omarsmak omarsmak Dec 16, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer if we don't use the underscore. Just to keep it consistent with what we have in the codebase

import org.apache.http.entity.FileEntity;
import org.apache.http.entity.InputStreamEntity;
import org.apache.http.entity.StringEntity;
import org.apache.http.entity.*;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same goes here and other places as well

producer.process(exchange);
producer.stop();

assertExchange(exchange);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If not mistaken, this is will not assert the custom headers, is that correct? (mind you I have no details info about this component)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Check HeaderValidationHandler I set a Map with expected Headers and when component creates a request it will send request to local server defined in test and validate Host header there. Please correct me if I am wrong. :) Thanks for taking the time to check this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah now it makes sense. I wondered before where the header assert is happening and I didn't pay attention to this part. Thanks for clarification :)

@djoleB
djoleB requested a review from omarsmak December 15, 2019 20:23

@omarsmak omarsmak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @djoleB, looks good to me so far, only the underscore naming in the tests if you can change and reverting back the imports

@djoleB
djoleB requested a review from omarsmak December 16, 2019 20:19

@omarsmak omarsmak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thanks @djoleB !

@omarsmak
omarsmak merged commit bce8c2c into apache:master Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants