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

Set Http protocol version #213

Merged
merged 1 commit into from
May 11, 2021
Merged

Conversation

mpp-jamesl
Copy link
Contributor

Change allows for setting the highest http version protocol. Defaults to version 1.1

Change allows for setting the highest http version protocol. Defaults to version 1.1
@ar ar merged commit ce8c779 into jpos:master May 11, 2021
HttpVersion definedVersion = null;
httpVersion = getVersion(ctx);
if(httpVersion != null && httpVersion.length() > 0) {
String[] versions = httpVersion.split(",");
Copy link
Member

Choose a reason for hiding this comment

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

I see you use a comma instead of a dot as the version separator. Wouldn't be better to use a dot as used by the HTTP protocol itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I wrestled with this for a bit of time, the option was to go with a (.) the usual given that we have say 1.1/2.0 etc. I opted for the comma on semantics. Conceptually I was thinking that we don't want to tie the major version to the minor version, the onus is on the developer to ensure that they are setting the values correctly not on jpos to check the values.

Copy link
Member

Choose a reason for hiding this comment

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

But are you OK if we change that to use a 'dot' and use split(".") instead of split(",") ? The rest of the code would be the same. It just feels more natural to set HTTP_VERSION to say "2.0" than "2,0".

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with using a dot such as "1.1"; it's the natural separator.

There are really just a few HTTP versions in the wild, and 97% of the time it's "1.1".

If we care enough to make it explicit for our project, in the published context entry HTTP_VERSION, it's because we know that we're going to be working with exactly that version, and there's no point in worrying about "tie[ying] the major version to the minor version" .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm very much ok with it, if you guys want to go with the dot separator then yes that's something I absolutely have no problems with that.

Copy link
Member

Choose a reason for hiding this comment

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

Excellent. Thank you. Will do that right away.

ar added a commit that referenced this pull request May 13, 2021
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.

3 participants