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

Fixed AbstractRequestService to modify parameters by the return value #1372

Closed
wants to merge 4 commits into from

Conversation

maediy
Copy link

@maediy maediy commented Dec 9, 2021

Currently, to modify the parameter, we have to modify the argument directly, not the return value.
But I think we should modify the parameter by the return value of ParameterCustomizer.

  • AS-IS
class MyParameterCustomizer implements ParameterCustomizer {
	@Override
	public Parameter customize(Parameter parameterModel, MethodParameter methodParameter) {
		parameterModel.setIn("path");
		parameterModel.setRequired(true);
		return parameterModel; // Anything is OK because this value will be ignored.
	}
}
  • TO-BE
class MyParameterCustomizer implements ParameterCustomizer {
	@Override
	public Parameter customize(Parameter parameterModel, MethodParameter methodParameter) {
		return new PathParameter();
	}
}

@maediy maediy force-pushed the bugfix/parameter-customizer branch from be4025d to 15f8b05 Compare December 9, 2021 10:14
@maediy maediy force-pushed the bugfix/parameter-customizer branch from 15f8b05 to 31e1e1d Compare December 9, 2021 10:34
@bnasslahsen
Copy link
Contributor

@maediy,

Thank you for your contribution.
Please add a unit test, that highlights how this PR solves/improves the existing behavior.

@bnasslahsen
Copy link
Contributor

@maediy,

I see your point. It would have been cleaner to get ParameterCustomizer.customize without any return (void).
This would get it much clear for your case. The idea of the customize is to act on the existing object.

We believe, that ParameterCustomizer can help you achieve what you want without any additional enhancements.
Thanks again for your contribution.

@maediy
Copy link
Author

maediy commented Dec 12, 2021

@bnasslahsen Thanks for reviewing my request.

I see your point. But how about customizing the parameter as invisible though.
I'm sorry that I didn't make such a test case but I still think the customizer should return the customized value and should use it in the situation to hide parameters with the customizer.

public class MyPathParameterCustomizer implements ParameterCustomizer {
    @Override
    public Parameter customize(Parameter parameterModel, MethodParameter methodParameter) {
        if (ShouldToBeIgnoredClass.isAssignableFrom(methodParameter.getParameterType())) {
            return null; // returning null means the parameter should be hidden
        }
        return parameterModel;
    }
}

And, in the case of app172 I wrote, the customizer changes a parameter from QueryParameter to PathParameter.
If there is no returning value, we will write as below,

public class MyPathParameterCustomizer implements ParameterCustomizer {
    @Override
    public void customize(Parameter parameterModel, MethodParameter methodParameter) {
        parameterModel.setIn("path");
        parameterModel.setRequired(true);
        parameterModel.setSchema(new StringSchema());
        // `parameterModel.getClass()` is still `QueryParameter`, not `PathParameter`
    }
}

but the type of the parameter is still QueryParameter. I think the type of the param should be changed to PathParameter as well.

@bnasslahsen
Copy link
Contributor

@maediy,

The type is Parameter. Since you set the In attribute, swagger-core handles correctly the Serialization of your object and will not be getting a wrong OpenApi spec.

@maediy
Copy link
Author

maediy commented Dec 12, 2021

@bnasslahsen
Oh, I'm sorry. That was my misunderstanding.
Yes, we can change it from query to path by changing In attribute.

How about hiding a parameter by returning null?

@bnasslahsen
Copy link
Contributor

ParameterCustomizer is not there to hide parameters.

Prefer the swagger annotations: @Hidden or @Parameter(hidden=true)
If not possible use OpenAPiCustomiser if you want hide parameter programmatically.

@maediy
Copy link
Author

maediy commented Dec 12, 2021

Got it. Thank you very much.

@bnasslahsen
Copy link
Contributor

@maediy,

I have made a change to make sure we can customize the operation and parameters by the return value.
I think this might interest you !

@KENNYSOFT
Copy link
Contributor

Yeah but not accepting null 😭

@bnasslahsen
Copy link
Contributor

@KENNYSOFT,

I guess, your test should be passing.
But if you have another use case, feel free to propose a PR!

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.

None yet

3 participants