Reactive doc points to unit tests #9157
Conversation
Reactive doc point to unit tests for further example of configuration
gberche-orange
changed the title
spring-projects-issues
added
the
status: waiting-for-triage
There was a problem hiding this comment.
Thanks, @gberche-orange for the PR! I've left my feedback inline.
jzheaux
added
in: docs
|
Thanks @jzheaux for your feedback. I've added a section about webflux support for multiple filter, inspired by my project at https://github.com/orange-cloudfoundry/osb-reverse-proxy/blob/6e38440c53992b343606c872375c1a087c8ccb40/src/main/java/com/orange/oss/osbreverseproxy/SecurityConfig.java#L99-L151 which contains a bit more background which I trimmed from the contributed sample. As a disclaimer, I'm still pretty new to spring-security, and may benefit from guidance/feedback on how to simplify this configuration. |
There was a problem hiding this comment.
Thanks for the changes, @gberche-orange! I've left some additional feedback inline.
| @@ -124,3 +124,66 @@ class HelloWebfluxSecurityConfig { | |||
|
|
|||
| This configuration explicitly sets up all the same things as our minimal configuration. | |||
| From here you can easily make the changes to the defaults. | |||
|
|
|||
| You can find more examples of explicit configuration in unit tests, by searching https://github.com/spring-projects/spring-security/search?q=path%3Aconfig%2Fsrc%2Ftest%2F+EnableWebFluxSecurity[EnableWebFluxSecurity in the `config/src/test/` directory], e.g. https://github.com/spring-projects/spring-security/blob/9cf3129d7afa2abb439aba6aadfee0a2c8c784bf/config/src/test/java/org/springframework/security/config/annotation/web/reactive/EnableWebFluxSecurityTests.java#L349-L366[MultiSecurityHttpConfig] illustrating multiple `SecurityWebFilterChain` beans, which is also explained in section below. | |||
There was a problem hiding this comment.
Even with the new section, I still think that this sentence should stay focused on one thing and not two. Could we simplify it to:
You can find more examples of explicit configuration in unit tests, by searching https://github.com/spring-projects/spring-security/search?q=path%3Aconfig%2Fsrc%2Ftest%2F+EnableWebFluxSecurity[EnableWebFluxSecurity in the `config/src/test/` directory].
There was a problem hiding this comment.
I felt that while unit tests are a useful workaround for lack of documentation, they are still hard to grasp for the users of a library. I feel that pointing to a specific test explained in more details in a dedicated session lowers the barrier to reading unit tests by users.
I've however removed the MultiSecurityHttpConfig as requested.
following #9157 (review) - added anchor - removed explicit reference to `MultiSecurityHttpConfig` - replaced multi security example with extracts from `MultiSecurityHttpConfig` and samples/boot/webflux-form/src/main/java/sample/WebfluxFormSecurityConfig.java as `MultiSecurityHttpConfig` isn't using explictly two SecurityWebFilterChain but seems to rely on defaults instead
|
Thanks @jzheaux for your additional review. I've refined the PR the best I could It is quite challenging for me as a newcomer and 1st time contributor to reverse engineer documentation from unit tests and to come up with documentation that match quality that that core maintainers would produce. If there are significant improvements required to this PR, then feel free to then to use this PR as a starting point, and consider further improving it to match the expected documentation quality. |
|
Thanks for the PR, @gberche-orange! This is now merged into Also, I polished it the new section with 8b7751f to update some of the indentation and standardize the explanations to fit the typical writing style of the reference manual. |
|
Great, thanks @jzheaux ! |
As a spring security user
This PR adds to the reactive documentation pointer to unit tests as further examples of configurations.
It might be possible to user asciidoc syntax to directly source the
MultiSecurityHttpConfiginstead of pointing to github (but this falls outside of my asciidoc so far). This may make maintenance of this section overtime easier if ever tests would detect broken path whenMultiSecurityHttpConfigsource code gets refactored.