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

Introduce Environment.matchesProfiles() for profile expressions #30206

Closed
garretwilson opened this issue Mar 27, 2023 · 6 comments
Closed

Introduce Environment.matchesProfiles() for profile expressions #30206

garretwilson opened this issue Mar 27, 2023 · 6 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

@garretwilson
Copy link

garretwilson commented Mar 27, 2023

I noticed that you've deprecated Environment.acceptsProfiles(String... profiles) in favor of a method that takes a Profile instance.

I can guess at the motivations behind this, but you might not have considered that this will cause problems in environments where it's easier to work with strings. (After all, profiles start out identified by strings.) For example, in Thymeleaf I might want to only include something if the active profile is production, as explained on Stack Overflow:

<div th:if="${#arrays.contains(@environment.getActiveProfiles(),'production')}">
     This is the production profile
</div>

If you eventually remove Environment.acceptsProfiles(String... profiles), this sort of thing will become more difficult.

Can you suggest an alternative in Thymeleaf?

Otherwise, you might want to reconsider deprecating this method.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Mar 27, 2023
@rstoyanchev rstoyanchev added the in: core Issues in core modules (aop, beans, core, context, expression) label Mar 28, 2023
@sbrannen sbrannen changed the title Deprecating Environment.acceptsProfiles(String... profiles) makes it difficult for Thymeleaf. Deprecating Environment.acceptsProfiles(String...) makes it difficult for Thymeleaf Mar 28, 2023
@sbrannen
Copy link
Member

Indeed, Environment.acceptsProfiles(String...) has been deprecated since Spring Framework 5.1; however, we have not yet indicated an intent to remove it in a particular release.


In any case, a quick search online reveals that you should be able to use th:with to invoke static methods like org.springframework.core.env.Profiles.of(String...).

@garretwilson, does the following work for you?

<div
  th:with="profiles=${T(org.springframework.core.env.Profiles).of('production')}" 
  th:if="${@environment.acceptsProfiles(profiles)}">
     This is the production profile
</div>

@sbrannen sbrannen added the status: waiting-for-feedback We need additional information before we can continue label Mar 28, 2023
@garretwilson
Copy link
Author

garretwilson commented Mar 28, 2023

Hi. Thanks for the quick response. For my simple page (providing debug information), the deprecated method was working. I posted this ticket not so much to help me, but to help the community by notifying the Spring team of implications of deprecation.

As for my case, I'll have to try the example you gave. I'm not sure when I'll be back in that area of code. But sincerely, it looks a bit ugly and verbose. It's getting close to including actual code in my Thymeleaf page, which we want to stay away from. Needing to create a Profile value class instance from the template negates some of the benefits of the template.

Sure, maybe Thymeleaf needs to create a facility to recognize value classes with .of(String) static factory methods and recognize them in constructors and wrap them automatically. But that's quite a task and would need to be pushed down into their expression language library. Who knows when that would or could happen.

As for me I'll just continue to use the deprecated approach for now, especially if you don't intend to remove it. I just prefer not to use deprecated methods. Here I may remove it for production anyway.

But I could imagine many circumstances in which I would want to do something in Thymeleaf based upon the profile. I wouldn't want to advocate that the team use deprecated methods. But I also wouldn't be really happy if they started sticking in that huge chunk of logic (which they would invariably have to look up anyway; no one could remember it without practice) just to check the profile. You see what I mean?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 28, 2023
@sbrannen sbrannen added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Mar 29, 2023
@sbrannen sbrannen self-assigned this Mar 29, 2023
@sbrannen sbrannen added this to the 6.0.8 milestone Mar 29, 2023
@sbrannen
Copy link
Member

sbrannen commented Mar 29, 2023

Hi @garretwilson,

Thanks for the feedback, and yes, I see what you mean.

When I posted that "workaround", I knew it was ugly and not user-friendly. My goal was to determine if it would be possible to achieve the same thing in Thymeleaf (and other similar scenarios) if we did actually remove that deprecated method at some point.

Regarding the deprecated method, it was deprecated in 5.1 in conjunction with #17063. The original (now deprecated) method only supports OR semantics; whereas, the new method supports profile expressions. Thus, the goal was to encourage people to use the more powerful profile expressions instead of the limited OR support.

That remains the goal, and in light of that I plan to introduce a method similar to the following in Environment.

default boolean acceptsProfiles(String profileExpression) {
	return acceptsProfiles(Profiles.of(profileExpression));
}

That should then support either of the following scenarios in Thymeleaf.

Single Profile

<div th:if="${@environment.acceptsProfiles('production')}">
     This is the production profile
</div>

Profile Expression

<div th:if="${@environment.acceptsProfiles('production & cloud')}">
     This is the production 'profile and cloud' profile
</div>

I am therefore repurposing this issue to introduce the new acceptsProfiles(String profileExpression) default method in Environment.

@sbrannen sbrannen changed the title Deprecating Environment.acceptsProfiles(String...) makes it difficult for Thymeleaf Introduce Environment.acceptsProfiles() variant that supports profile expressions Mar 29, 2023
@sbrannen sbrannen changed the title Introduce Environment.acceptsProfiles() variant that supports profile expressions Introduce Environment.acceptsProfiles() variant for profile expressions Mar 29, 2023
@sbrannen sbrannen added the for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x label Mar 29, 2023
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x labels Mar 29, 2023
@sbrannen sbrannen modified the milestones: 6.0.8, 6.0.9 Apr 11, 2023
@sbrannen sbrannen changed the title Introduce Environment.acceptsProfiles() variant for profile expressions Introduce Environment.matchesProfiles() for profile expressions Apr 25, 2023
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Apr 25, 2023
@sbrannen
Copy link
Member

In the end we decided to introduce the following method in Environment to provide explicit support for profile expressions provided directly as strings.

	default boolean matchesProfiles(String... profileExpressions) {
		return acceptsProfiles(Profiles.of(profileExpressions));
	}

This change has been applied to 6.0.x and main and will be backported to 5.3.x.

sbrannen added a commit to sbrannen/spring-framework that referenced this issue Apr 25, 2023
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Apr 25, 2023
Environment.acceptsProfiles(String...) was deprecated in 5.1 in
conjunction with spring-projectsgh-17063 which introduced a new
acceptsProfiles(Profiles) method to replace it. The deprecated method
only supports OR semantics; whereas, the new method supports profile
expressions. Thus, the goal was to encourage people to use the more
powerful profile expressions instead of the limited OR support with
profile names.

However, there are use cases where it is difficult (if not impossible)
to provide a Profiles instance, and there are use cases where it is
simply preferable to provide profile expressions directly as strings.

To address these issues, this commit introduces a new matchesProfiles()
method in Environment that accepts a var-args list of profile
expressions.

See spring-projectsgh-30206
Closes spring-projectsgh-30226
mdeinum pushed a commit to mdeinum/spring-framework that referenced this issue Jun 29, 2023
mdeinum pushed a commit to mdeinum/spring-framework that referenced this issue Jun 29, 2023
Environment.acceptsProfiles(String...) was deprecated in 5.1 in
conjunction with spring-projectsgh-17063 which introduced a new
acceptsProfiles(Profiles) method to replace it. The deprecated method
only supports OR semantics; whereas, the new method supports profile
expressions. Thus, the goal was to encourage people to use the more
powerful profile expressions instead of the limited OR support with
profile names.

However, there are use cases where it is difficult (if not impossible)
to provide a Profiles instance, and there are use cases where it is
simply preferable to provide profile expressions directly as strings.

To address these issues, this commit introduces a new matchesProfiles()
method in Environment that accepts a var-args list of profile
expressions.

Closes spring-projectsgh-30206
mdeinum pushed a commit to mdeinum/spring-framework that referenced this issue Jun 29, 2023
@garretwilson
Copy link
Author

I just confirmed this works in Spring Boot 3.1.2 and converted to using the new facility. Thanks!

@sbrannen
Copy link
Member

sbrannen commented Aug 2, 2023

I just confirmed this works in Spring Boot 3.1.2 and converted to using the new facility.

Glad to hear it. Thanks for the feedback.

Thanks!

You're welcome!

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