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

Non-public property in Saml2RelyingPartyProperties' Registration class #19194

Closed
sbattistin opened this issue Nov 29, 2019 · 6 comments
Closed
Assignees
Labels
type: bug A general bug
Milestone

Comments

@sbattistin
Copy link

sbattistin commented Nov 29, 2019

I was trying to use the properties set in the YAML file through the Registration class inside Saml2RelyingPartyProperties class.

The problem is that the method has only package visibility and is not public.

		public Signing getSigning() {
			return this.signing;
		}

-->		Identityprovider getIdentityprovider() {
			return this.identityprovider;
		}

		public static class Signing {

P.S. I'm not sure if there is a reason to not make this public.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 29, 2019
@sbattistin
Copy link
Author

I opened this PR if needed: #19195

@wilkinsona
Copy link
Member

As noted on #19195, @ConfigurationProperties classes are not considered to be public API. We should use this issue to address the inconsistent visibility of the getters but the result may be that getSigning() is made package-private rather than getIdentityprovider being made public.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Nov 29, 2019
@woodforda
Copy link

woodforda commented Nov 30, 2019

Hi, just a comment regarding use cases for using the internal config properties classes. I too have stumbled yesterday into this visibility issue. I have a use case where the powers decreed that all config must be in TypeSafe format (sadly) not yml/properties. So I have an EnvironmentPostProcessor to add the property sources for the TypeSafe config in the correct order. So far so good. My issue comes when I try to let Boot Auto config properties. Simple primitive bindings work ok. However, trying to bind an object (like a list of registrations) doesn't work (I end up with a list of a map and later a class cast exception). I therefore need to transform the properties to the config class in my bean config class either overriding or using @Primary (for just the config property class). Sadly, this is not possible due to the visibility.

The idea for doing it like this comes directly from the Boot documentation (2 data source set up) where we do need to use the internal config prop classes... ?

@wilkinsona
Copy link
Member

wilkinsona commented Nov 30, 2019

@woodforda Thanks for the additional use case. That sounds like something isn’t quite right in your environment post-processor. If you map the keys in the same way as Boot does for YAML or properties files, the binding should work without the need for any further transformation. I’d recommend comparing the contents of the property sources when you have equivalent TypeSafe or YAML/properties configuration to help to diagnose the problem.

I’ve opened #19199 to straighten out the contradiction in the documentation. Thanks for pointing it out.

@woodforda
Copy link

woodforda commented Nov 30, 2019

@wilkinsona You're welcome. Yep I understand the issue - just trying to ignore the the inevitable - Spring would happily convert all for me but this happens (I assume) as you populate the property source - the TypeSafe puts, by default, a Map in. So I would have to write a converter in the TypeSafe world to do it and put the result in the property source. But would like to avoid coding something that Spring would do out of the box. I suspect I have not many choices. However, not quite the topic of this ticket :) but thanks!

@philwebb philwebb added type: bug A general bug and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Dec 13, 2019
@philwebb philwebb added this to the 2.2.x milestone Dec 13, 2019
@snicoll
Copy link
Member

snicoll commented Dec 30, 2019

We should use this issue to address the inconsistent visibility of the getters but the result may be that getSigning() is made package-private rather than getIdentityprovider being made public.

@ConfigurationProperties are public across the codebase as far as I know. Marking it package private is a signal for the annotation processor to determine this isn't a property to generate metadata for. In this very case, Registration is the value of a Map so no metadata is generated at this point. For consistency, I think it should be flagged public.

That doesn't mean this API is meant to be called directly, as documented in the reference documentation (updated).

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

No branches or pull requests

6 participants