Closed mustaphazorgati closed 3 years ago
Thanks for the submition, let's discuss details on the PR.
Hi @mustaphazorgati. I just looked into this again and realised what you are trying to achieve is actually a support for ModelAttribute without annotation. If I'm getting this right, from documentation of method arguments:
Any other argument If a method argument is not matched to any of the earlier values in this table and it is a simple type (as determined by BeanUtils#isSimpleProperty, it is a resolved as a @RequestParam. Otherwise, it is resolved as a @ModelAttribute.
We already have a support for @ModelAttribute annotation via JacksonModelAttributeSnippet. See controller and test. This one could theoretically work without annotation if I look at the implementation logic.
So what I would propose you to do:
If none of this will work, or will work in a different than intended way, please share your results with us.
Hey @jmisur, thank you for your response. Indeed The JacksonModelAttributeSnippet was what we were looking for. Unfortunately that Snippet only resolves the ModelAttribute DTO. Since we have multiple ModelAttribute DTOs I extended that snippet to recognize multiple ModelAttributes. That change is backwards compatible and does not break anything. Based on this change we could further customize the Snippets for multiple Types. Furthermore I also expanded the JacksonRequestFieldSnippet in the same way.
Are you able to check master on your project?
I'll grab the newest snapshot version and verify it :) talk to you shortly
Everything works like a charm! When will we get a new release?
Not so fast. I regenerated documentation, and now it's outputting also specialised DTOs like Pageable
as modelattributes. I'm afraid that it catches all non-annotations and non-simple-props according to docs , e.g. Errors
, ServletRequest
, HttpMethod
, Principal
, etc.
good point.
Suggestion: We remove the support for non-annotated parameters and enforce the user to explicitly use the @ModelAttribute
annotation?
I think that's the lesser pain than to support the catch according to the docs and implement a mechanism to explicitly exclude spring related classes.
If you agree I'll create a new PR shortly.
Yes I agree. Let's rollback the last PR (to some extent) and just request users to use the annotation. It's the simplest and not really an intrusive approach.
Alright. PR coming later today.
Hey @jmisur,
I have a little issue. After reverting the isModelAttribute
method back to verifying that the ModelAtrribute
annotation is present the bahviour does not change. This is because the method isProcessedAsModelAttribute
(introduced in #393) always returns true which causes every DTO (without annotation) to be accepted as a type.
I think that was not an issue until now, because the previous behavior stopped after getting the first Type. Now the snippet returns multiple types if multiple annotated parameters exist. Can you help me out here?
HttpMethod
will not be documented, but Pageable
will be.
Hey @jmisur,
did you have a chance to look at this?
I did but I need to dig a bit deeper into this.
If I can help somehow let me know. :)
I think I got it ^^.
Hey @jmisur! That's great to hear! - Thank you for your effort :blush:
I had a look through your PR and I am a little confused.
As far as I can tell you removed the isModelAttribute
method and fix some tests.
What am I missing? How does this fix the faulty documentation generation?
That method was a devil in disguise. It was basically performing a supports function of ModelAttributeMethodProcessor
. However it was not supposed to do it.
What we're doing now is just delegating the resolution of supporting the current argument onto the chain of handlerMethodArgumentResolvers
, which determine what kind of attribute it is and if we support it. This strictly follows the documentation about processing method arguments and keeps the order intact. Because ModelAttributeMethodProcessor
is usually the last one in the chain, it will also evaluate for the annotation and everything else not supported by previous handlers.
Thank you for the explanation! :) I am glad this issue is finally done
Thank you for the explanation! :) I am glad this issue is finally done
Me too. Took just a couple of months... 🙈 2.0.10 is out.
Ah well. We got there eventually. Thanks for the update!
Because we have too many request parameter for some endpoints we decided to wrap them into dedicated DTO like so:
Unfortunately this way the code looks clean, but the request parameters which are contained within each DTO are not documented.
Is this currently not supported or are we doing something wrong?