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

server.reactive.session.cookie properties are not documented #31896

Closed
4 tasks done
wilkinsona opened this issue Jul 27, 2022 · 20 comments
Closed
4 tasks done

server.reactive.session.cookie properties are not documented #31896

wilkinsona opened this issue Jul 27, 2022 · 20 comments
Assignees
Labels
status: first-timers-only An issue that can only be worked on by brand new contributors status: superseded An issue that has been superseded by another type: documentation A documentation update

Comments

@wilkinsona
Copy link
Member

wilkinsona commented Jul 27, 2022

Hi, this is a first-timers-only issue. This means we've worked to make it more understandable to people who haven't contributed to our codebase before and are just getting started with making open source contributions in general.

If that's you, we're interested in helping you take the first step and can answer questions and help you out as you do. Those questions can be about the code, working with Git and GitHub – anything that you need some help with to make the contribution. Note that we're especially interested in contributions from people from groups underrepresented in free and open source software!

If you have had a pull request merged before, consider leaving this one for someone new, and looking through our general ideal-for-contribution issues. Thanks!

Problem

@DidierLoiseau has pointed out that the server.reactive.session.cookie properties are not documented. This means that they aren't listed in the reference documentation or in the metadata that enables auto-completion in an IDE when editing application.properties and application.yaml files.

The entries in the reference documentation are generated from the metadata so it's the metadata that needs to be corrected. It is generated using an annotation processor. The properties are defined on a Cookie class, an instance of which is used by ServerProperties.Reactive.Session:

The annotation processor doesn't know that Cookie contains nested configuration properties so no metadata is generated.

Solution

org.springframework.boot.autoconfigure.web.ServerProperties.Reactive.Session should be updated to add @NestedConfigurationProperties to its cookie field. The source for this class is in ServerProperties.java that can be found at spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java in your git clone of the codebase.

The reference documentation can then be built (./gradlew spring-boot-project:spring-boot-docs:asciidoctor) to check that the property is now documented. The built documentation can be found beneath spring-boot-project/spring-boot-docs/build/docs/.

Steps to Fix

  • Claim this issue with a comment below and ask any clarifying questions you need
  • Set up a repository locally following the Contributing Guidelines
  • Try to fix the issue following the steps above
  • Commit your changes and start a pull request.
@wilkinsona wilkinsona added this to the 2.6.x milestone Jul 27, 2022
@wilkinsona wilkinsona added the type: documentation A documentation update label Jul 27, 2022
@wilkinsona wilkinsona added the status: first-timers-only An issue that can only be worked on by brand new contributors label Jul 27, 2022
@spring-projects spring-projects deleted a comment from weixsun Jul 27, 2022
@hezean
Copy link

hezean commented Jul 27, 2022

Hi, can you give me a chance to work on it? Many thanks!

@wilkinsona
Copy link
Member Author

Thanks very much for the offer, @hezean. Looking at your GitHub profile, you appear to be an experienced contributor already. If you don't mind, we'd prefer that someone who's just getting started in open source has an opportunity to claim the issue.

@hezean
Copy link

hezean commented Jul 27, 2022

Never mind, I'll look for other issues and try to start contributing to spring boot :)

@Adyel
Copy link

Adyel commented Jul 28, 2022

Hello @wilkinsona 👋,

Do you think I am eligible for this first-timers-only Issue? I would love to be involved in Spring development and this seems like a easy and approachable issue to start with.

@wilkinsona
Copy link
Member Author

Thanks very much for the offer, @Adyel. We believe that fixing this issue will require a small change to a single class so it's not ideal for learning about Spring development. It's more suited to someone who wants to gain some experience of using GitHub and the pull request process. With 60 contributions in the last year, you appear to already be quite experienced in that area. If you don't mind, we'd prefer that it's claimed by someone who's really just getting started.

Please look out for issues labeled as ideal for contribution (we don't have any at the moment unfortunately) that are a better fit for someone who's contributed to OSS before but perhaps not to Spring.

@jakubskalak

This comment was marked as resolved.

@snicoll

This comment was marked as resolved.

@zantoichi
Copy link

zantoichi commented Jul 28, 2022

Hello @wilkinsona

I think I can tackle this if you want me to try!

@wilkinsona
Copy link
Member Author

Thanks for the offer, @zantoichi. I can see that you've already had a pull request merged. If you don't mind, we'd really like to invest our time in helping someone who hasn't been through the pull request process before.

Please look out for issues labeled as ideal for contribution (we don't have any at the moment unfortunately) that are a better fit for someone who's contributed to OSS before but perhaps not to Spring.

@Kalpesh-18
Copy link
Contributor

Hey @wilkinsona am I eligible to work on it?

@wilkinsona
Copy link
Member Author

@Kalpesh-18 It doesn't look like you've opened a pull request before, so yes you are. Thanks very much. I'll assign the issue to you. If you have any questions at all, please don't hesitate to ask.

@rajseth88
Copy link

Even I am not well versed with raising PR and cloning such large project repo in my local,could you please also assign me some task?

@Kalpesh-18
Copy link
Contributor

Thank you @wilkinsona will work on it

@wilkinsona
Copy link
Member Author

@rajseth88 Thanks for the offer. We don't have anything suitable right now, but please keep a look out for first-timers-only issues in the future.

@Kalpesh-18
Copy link
Contributor

Hey @wilkinsona while importing it as a gradle project, should I import the entire spring-boot or just spring-boot-autoconfigure

@wilkinsona
Copy link
Member Author

I would import everything by pointing your IDE at the root of your local git clone of the code.

@DidierLoiseau
Copy link

Thanks a lot for the reactivity!

In addition to the properties themselves, I think it would be useful to have a mention of those properties in the documentation itself to make it more easily discoverable. Section 8.5 Spring Session already mentions server.servlet.session.timeout but not the cookie properties, nor the equivalent reactive properties. Section 1.3.4. Customizing Embedded Servlet Containers mentions the cookie properties (and has a dedicated subsection specifically for the same-site property) but of course it only applies to servlet containers (might be worth linking the 2 sections though).

Also note that even for the servlet cookie properties, the JavaDoc is not copied into the generated documentation.

@wilkinsona
Copy link
Member Author

As this is a first-timers-only issue, we want to keep the scope quite narrow. If further changes are necessary, we'll open another issue to tackle those. Thanks for the suggestions.

@wilkinsona
Copy link
Member Author

Closing in favor of #31912.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Jul 29, 2022
@wilkinsona wilkinsona removed this from the 2.6.x milestone Jul 29, 2022
@wilkinsona wilkinsona added the status: superseded An issue that has been superseded by another label Jul 29, 2022
@wilkinsona
Copy link
Member Author

wilkinsona commented Jul 29, 2022

I've opened #31916 for the missing descriptions in the appendix. It's more than just the cookie properties that are affected. I've also opened #31917 to add some docs about the properties that are available to customize embedded reactive servers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: first-timers-only An issue that can only be worked on by brand new contributors status: superseded An issue that has been superseded by another type: documentation A documentation update
Projects
None yet
Development

No branches or pull requests

9 participants