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

Sort parameters by their order before sorting them by their name #2721

Merged
merged 1 commit into from
Aug 3, 2019

Conversation

shartte
Copy link

@shartte shartte commented Oct 6, 2018

Rather than sorting parameters only by their name, sort them by their existing order field first, then use the name as a tiebreaker.
Parameter already implements Ordered, so this seems like a natural way of sorting parameters.
This also enables plugins to apply any custom parameter order they want, while the current sorting prevents that.
Currently parameters all have the initial order value of Integer.MIN_VALUE, so this change should not affect the current ordering without any additional plugins applied.

@codecov
Copy link

codecov bot commented Oct 6, 2018

Codecov Report

Merging #2721 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2721      +/-   ##
============================================
+ Coverage     94.77%   94.77%   +<.01%     
- Complexity     3161     3162       +1     
============================================
  Files           354      354              
  Lines          8057     8058       +1     
  Branches        608      608              
============================================
+ Hits           7636     7637       +1     
  Misses          271      271              
  Partials        150      150
Impacted Files Coverage Δ Complexity Δ
...ava/springfox/documentation/service/Operation.java 100% <100%> (ø) 21 <1> (+1) ⬆️

@mikecollard
Copy link

+1

@dilipkrish dilipkrish added the PR label Nov 7, 2018
@dilipkrish dilipkrish added this to the 3.0 milestone Nov 7, 2018
@raderio
Copy link

raderio commented Dec 10, 2018

+1

@ratoaq2
Copy link

ratoaq2 commented Jan 16, 2019

+1

I was able to keep the parameters order just adding this plugin alongside the change of this PR

import org.springframework.core.Ordered;
import org.springframework.core.annotation.Order;
import org.springframework.stereotype.Component;

import springfox.documentation.service.ResolvedMethodParameter;
import springfox.documentation.spi.DocumentationType;
import springfox.documentation.spi.service.ParameterBuilderPlugin;
import springfox.documentation.spi.service.contexts.ParameterContext;
import springfox.documentation.swagger.common.SwaggerPluginSupport;

@Component
@Order(SwaggerPluginSupport.SWAGGER_PLUGIN_ORDER)
public class ParameterIndexOrderReader implements ParameterBuilderPlugin {

    private static final int PARAMETER_INITIAL_ORDER = Ordered.HIGHEST_PRECEDENCE;

    @Override
    public boolean supports(DocumentationType delimiter) {
        return true;
    }

    @Override
    public void apply(ParameterContext context) {
        ResolvedMethodParameter methodParameter = context.resolvedMethodParameter();
        context.parameterBuilder().order(PARAMETER_INITIAL_ORDER + methodParameter.getParameterIndex());
    }
}

@ratoaq2
Copy link

ratoaq2 commented Jan 16, 2019

The change in this PR doesn't affect the current 2.9.x behavior (alphabetically sorted)

But IMO, the parameter index should be kept and that should be the default behavior. The API spec is used to auto generate code and sorting it alphabetically by default doesn't seem reasonable.

@ratoaq2
Copy link

ratoaq2 commented Jan 16, 2019

As a side note, OperationBuilder, ParameterBuilder and similars could be spring components with scope 'prototype' so we could easily override them

@tholu
Copy link

tholu commented May 31, 2019

@shartte Can you resolve the conflict? I would also like to keep the defined order instead of sorting parameters alphabetically and am looking forward to see this landing in 3.0.

@shartte
Copy link
Author

shartte commented May 31, 2019

Sorry, no, I won't put any more time in. It seems like this PR has no chance of being merged given how long it is open.

@dilipkrish dilipkrish merged commit 994f903 into springfox:master Aug 3, 2019
@dilipkrish
Copy link
Member

@shartte sorry it took so long. I've resolved the conflicts and merged.

@dilipkrish
Copy link
Member

Thanks for the PR! 🤘

@antechrestos
Copy link

Hi,

@dilipkrish Will it be released soon? This change is quite usefull INMO.

@dilipkrish
Copy link
Member

@antechrestos yes Im trying to as soon as I can... see #3070

@schrek1
Copy link

schrek1 commented Jul 14, 2020

looks still broken, I not able forced order instead alphabetical sorting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants