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

Profiles should be comparable when created via Profiles.of() #25340

Closed
stsypanov opened this issue Jun 30, 2020 · 3 comments
Closed

Profiles should be comparable when created via Profiles.of() #25340

stsypanov opened this issue Jun 30, 2020 · 3 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Milestone

Comments

@stsypanov
Copy link
Contributor

stsypanov commented Jun 30, 2020

Suppose we have this code in production:

private boolean isExternalProfileEnabled() {
  return environment.acceptsProfiles(Profiles.of("external"));
}

Then I want to mock this in tests and I write something like:

when(environment.acceptsProfiles(Profiles.of("external"))).thenReturn(true);

And at runtime the mock fails to return true, because comparison of arguments fails due to missing equals() and hashCode() implementations in ParsedProfiles.

This makes us use the deprecated API and rewrite the example code like:

private boolean isExternalProfileEnabled() {
  return environment.acceptsProfiles("external");
}

and use of the deprecated API in turn makes Sonar unhappy.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 30, 2020
@sbrannen sbrannen self-assigned this Jul 6, 2020
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jul 6, 2020
@sbrannen sbrannen added this to the 5.2.8 milestone Jul 6, 2020
@jhoeller
Copy link
Contributor

jhoeller commented Jul 6, 2020

@sbrannen Let's backport this to 5.1.x as well since that specific Profiles API got introduced in 5.1.

@jhoeller jhoeller added the for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x label Jul 6, 2020
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x labels Jul 6, 2020
@sbrannen
Copy link
Member

sbrannen commented Jul 6, 2020

@jhoeller, I was literally just about to discuss this with you.

It turns out it's not as simple as it may appear at a quick glance.

I've experimented locally but haven't checked anything into a branch.

In summary...

In order to properly implement equals() and hashCode() we might have to break down the individual expressions using something like an AST in order to determine the equivalence between logically equivalent expressions such as spring & java and java & spring. The same applies for ! and | support.

Currently, ParsedProfiles stores each expression as a single String. So we cannot reliably use that in any robust way. It works if the tokens of the expressions are specified in the same order. For example, spring&java and spring & java can easily be checked to be equivalent (ignoring all whitespace -- and I have that working locally); whereas, without analyzing the meaning of the expression, spring & java and java & spring cannot be determined to be logically equivalent. It's even more complex than that due to the tree-based structure of expressions (e.g., (spring & framework) | (spring & java)).

Now... the information is present in the internal Profiles[] parsed field, but that's not a data structure that we can query. The lambda expression returned by org.springframework.core.env.ProfilesParser.equals(String) captures the individual token (e.g., "spring" or "java"). So again the information is there, but not in any form that we can access for comparison.

Let's discuss our options...

@jhoeller
Copy link
Contributor

jhoeller commented Jul 6, 2020

Maybe we should simply restrict it to literal equality of the input String... probably good enough for the mocking scenario here. We could even preserve whitespace in the comparison, avoiding any special processing of the String for comparison purposes.

@sbrannen sbrannen changed the title Profiles should be comparable to be used with Mockito correctly Profiles should be comparable when created via Profiles.of() Jul 7, 2020
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Jul 7, 2020
Prior to this commit, a Profiles instance created via Profiles.of() was
not considered equivalent to another Profiles instance created via
Profiles.of() with the exact same expressions. This makes it difficult
to mock invocations of Environment#acceptsProfiles(Profiles) -- for
example, when using a mocking library such as Mockito.

This commit makes Profiles instances created via Profiles.of()
"comparable" by implementing equals() and hashCode() in ParsedProfiles.

Note, however, that equivalence is only guaranteed if the exact same
profile expression strings are supplied to Profiles.of(). In other
words, Profiles.of("A & B", "C | D") is equivalent to
Profiles.of("A & B", "C | D") and Profiles.of("C | D", "A & B"), but
Profiles.of("X & Y") is not equivalent to Profiles.of("X&Y") or
Profiles.of("Y & X").

Closes spring-projectsgh-25340
FelixFly pushed a commit to FelixFly/spring-framework that referenced this issue Aug 16, 2020
Prior to this commit, a Profiles instance created via Profiles.of() was
not considered equivalent to another Profiles instance created via
Profiles.of() with the exact same expressions. This makes it difficult
to mock invocations of Environment#acceptsProfiles(Profiles) -- for
example, when using a mocking library such as Mockito.

This commit makes Profiles instances created via Profiles.of()
"comparable" by implementing equals() and hashCode() in ParsedProfiles.

Note, however, that equivalence is only guaranteed if the exact same
profile expression strings are supplied to Profiles.of(). In other
words, Profiles.of("A & B", "C | D") is equivalent to
Profiles.of("A & B", "C | D") and Profiles.of("C | D", "A & B"), but
Profiles.of("X & Y") is not equivalent to Profiles.of("X&Y") or
Profiles.of("Y & X").

Closes spring-projectsgh-25340
zx20110729 pushed a commit to zx20110729/spring-framework that referenced this issue Feb 18, 2022
Prior to this commit, a Profiles instance created via Profiles.of() was
not considered equivalent to another Profiles instance created via
Profiles.of() with the exact same expressions. This makes it difficult
to mock invocations of Environment#acceptsProfiles(Profiles) -- for
example, when using a mocking library such as Mockito.

This commit makes Profiles instances created via Profiles.of()
"comparable" by implementing equals() and hashCode() in ParsedProfiles.

Note, however, that equivalence is only guaranteed if the exact same
profile expression strings are supplied to Profiles.of(). In other
words, Profiles.of("A & B", "C | D") is equivalent to
Profiles.of("A & B", "C | D") and Profiles.of("C | D", "A & B"), but
Profiles.of("X & Y") is not equivalent to Profiles.of("X&Y") or
Profiles.of("Y & X").

Closes spring-projectsgh-25340
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants