Prefer doIndex over index views #101
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For uniformity and to precisely control its priority, this logic should be a regular Dispatcher, not a hard-coded part of Stapler.tryInvoke()
This way code is more modular
... for better modularity. In the future we should allow this behaviour to be modified by the class, for example via annotation. Some API bound objects like Jenkins Blue Ocean would prefer not to have this behaviour.
So that the buildDispatchers method look a little manageable
Currently the code performs index view lookup before doIndex handling. This behaviour is inconsistent with the relative ordering between xyz.jelly vs doXyz where the latter wins. This behaviour is considered sensible because it'd allow programmatic check before a view is rendered by using StaplerRequest.getView(). The code also has a performance improvement effect for those model objects that do not define any index view but doIndex, by going straight to doIndex without bothering to try index views. Objects that do define index view without doIndex will perform the same, because the presence of doIndex is tested during MetaClass.buildDispatchers(), and not during dispatching requests. See: #97
A non-null ServletContext is needed to build MetaClass
This was referenced Jan 15, 2017
Merged
|
Tested with the tip of Jenkins to make sure all the tests pass |
|
@reviewbybees |
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
jglick
added a commit
that referenced
this pull request
Jan 18, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adjustment of pull request #97
The goal of #97 was to improve performance of request routing to objects that do not have index views but do have the
doIndexmethod. This is achieved by swapping the priority between those two routes, which was done in 8156216. This is strictly speaking an incompatible change, but in practice acceptable and preferrable because:doIndexmethod because the latter would have never been invokedTo make this possible, this PR also converts index view routing and a few other routes that were previously hard-coded in
Stapler.tryInvokeintoDispatcherimplementations.