Return type of oauth2.core.ClaimAccessor#containsClaim(String) could be a primitive boolean

[grimsa]

Current Behavior

In org.springframework.security.oauth2.core.ClaimAccessor interface there is a containsClaim method:

	/**
	 * Returns {@code true} if the claim exists in {@link #getClaims()}, otherwise {@code false}.
	 *
	 * @param claim the name of the claim
	 * @return {@code true} if the claim exists, otherwise {@code false}
	 */
	default Boolean containsClaim(String claim) {
		Assert.notNull(claim, "claim cannot be null");
		return getClaims().containsKey(claim);
	}

Return type of this method is a nullable Boolean.

Expected Behavior

It seems the method could return a non-nullable primitive boolean. This is supported by all of:

• javadoc - not mentioning null return value
• default implementation - Map#containsKey returning primitive boolean
• use in other methods assumes non-null values, e.g. return !containsClaim(claim) ? ... : ...

Context

Noticed this when SonarQube marked if (!accessTokenJwt.containsClaim(CUSTOM_CLAIM)) { code as non-compliant for rule Boxed "Boolean" should be avoided in boolean expressions.

[jgrandja]

@grimsa It's not clear to me if you are reporting an issue?

Return type of this method is a nullable Boolean.

null is never returned. It's always a boolean?

Which code are you referencing in if (!accessTokenJwt.containsClaim(CUSTOM_CLAIM))?

[grimsa]

@jgrandja I'm referencing ClaimAccessor#containsClaim (in the provided example accessTokenJwt is instance of ClaimAccessor).

My observation is - if null is never returned from containsClaim method, then why is the return type Boolean (which leads to false positives during static analysis) and not boolean?

It is definitely not a bug, just a possibility for minor enhancement.

[jgrandja]

@grimsa What enhancement are you proposing? Change return type from Boolean to boolean OR improve javadoc?

[grimsa]

Changing to boolean would be preferable.
If that is not possible, then Javadoc could be improved to explain why.

[jgrandja]

Changing return type to boolean would break compatibility for any 5.x version. We could update the javadoc. Would you be interested in submitting a PR for this?

[grimsa]

Sure. Let me know what the javadoc should be and I'll create a PR with that change.
Or maybe this should target 6.x instead?

[jgrandja]

@grimsa On second thought...

Let's @Deprecated:

Boolean containsClaim(String claim)

And add:

boolean hasClaim(String claim)

We should also ensure there are no reference to containsClaim() within the codebase.

Sound good?