Skip to content
This repository was archived by the owner on May 28, 2018. It is now read-only.

Conversation

@mads-b
Copy link

@mads-b mads-b commented Sep 4, 2014

This is a fix to reported issue JERSEY-2635, stemming from
the incorrect ordering of for loops in MethodSelectingRouter.

@jerseyrobot
Copy link
Contributor

Can one of the admins verify this patch?

@mads-b mads-b changed the title Produce first delared content-type on Accept:*/* Produce first declared content-type on Accept:*/* Sep 4, 2014
@mpotociar
Copy link
Collaborator

Hi, thank you for the contribution.

In order to accept your contribution, we need you to sign Oracle Contributor Agreement, see here for details: https://jersey.java.net/scm.html#/Submitting_Patches_and_Contribute_Code

@mpotociar
Copy link
Collaborator

Jenkins, please test this patch.

This is a fix to reported issue JERSEY-2635, stemming from
the incorrect ordering of for loops in MethodSelectingRouter.
@mpotociar
Copy link
Collaborator

Jenkins, please test this patch.

@mpotociar
Copy link
Collaborator

Hi, we're still waiting for your signed OCA. Have you sent it to us yet?

@mpotociar
Copy link
Collaborator

Seems like there is a problem with a copyright header in one of the changed test classes:
https://jersey-ci.ci.cloudbees.com/job/github-pull-request-verification/95/artifact/copyright.year.wrong.txt

@AdamLindenthal
Copy link
Member

Mads-b, could you please change the copyright header year in the ContentNegotiationTest to 2013-2014 instead of 2013 and re-commit?

Thanks!

@AdamLindenthal
Copy link
Member

Mads-b, are you still here? Are you planning to sign the OCA and fix the header or should we rather consider to commit internally?

Regards,
Adam

@mads-b
Copy link
Author

mads-b commented Nov 28, 2014

I am truly sorry for my extended silence. Tldr: my company is hesitant to sign such an agreement. If possible, I would appreciate it if this patch was merged internally. If it makes a difference, I do not need to retain any rights at all to this patch, I merely want it to be present in future Jersey releases.

@AdamLindenthal
Copy link
Member

Hi, thanks for letting us know. We can either close the pull request now, leave the JIRA issue still valid and look at the issue in one of the next sprints internally, or you can still consider signing the agreement yourself as an individual (this makes much more sense if you would plan to make further contributions in the future).

Just let us know once you have decided.
Thanks,
Adam

@mads-b
Copy link
Author

mads-b commented Nov 28, 2014

You can't copy this patch, make the necessary changes and merge it internally? I fixed this because we really needed this behaviour. I doubt I'll contribute again if no more severe bugs are found on our part.

@shamoh shamoh self-assigned this Jan 21, 2015
@shamoh shamoh added this to the 2.16 milestone Jan 21, 2015
@shamoh
Copy link
Contributor

shamoh commented Jan 21, 2015

Hi Mads.
I think we can take your change without your commit history.
I would like to ask what was the reason you changed methodBuilders type from Set to List? Was there some special reason?
Thanks,
Libor

@mads-b
Copy link
Author

mads-b commented Jan 21, 2015

I'm not entirely sure, since it was a while ago since I wrote this, but I recall fixing two bugs in succession: The ordering of the values in @produces annotation, but also an issue that surfaced after fixing this. For some reason, having multiple resource methods producing different media types results in random methods getting invoked when using the Accept: / header, when the topmost method should be invoked. The reason it works without the patch is pure coincidence, but after fix, the problem became more prominent.

@shamoh
Copy link
Contributor

shamoh commented Jan 21, 2015

I've been 'inspired' by your code. Fix will be in 2.16 release, I hope. Thanks again for your contribution.

@shamoh shamoh closed this Jan 21, 2015
shamoh added a commit that referenced this pull request Jan 27, 2015
…eader is "*/*""

Change inspired by github pull request #102 by @mads-b. Thanks.

Change-Id: Ib09187cfdfb6e118409554833197e31dbe1081db
mpotociar pushed a commit that referenced this pull request Jan 27, 2015
…cepts header is "*/*"" Change inspired by github pull request #102 by @mads-b. Thanks."
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants