Replace conversion to ObjectUtils.isEmpty with !StringUtils.hasLength

[dimijdriven]

What's changed?

StringUtils.isEmpty() is deprecated currently openrewrite replaces it with ObjectUtils.isEmpty() which is stricter
and led to this issue:

• StringUtils#isEmpty is not equivalent to ObjectUtils#isEmpty for collections #475

According to the official suggestions: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/util/StringUtils.html#isEmpty(java.lang.Object)

I replaced it with !StringUtils.hasLength

What's your motivation?

• Fixes StringUtils#isEmpty is not equivalent to ObjectUtils#isEmpty for collections #475

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

@Laurens-W

Have you considered any alternatives or workarounds?

Any additional context

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[Laurens-W]

This might be a bit broad

[timtebeek]

Also be sure to add a @RecipeDescriptor to the outer class, as that'll get turned into a Recipes class itself, which needs documentation.

[Laurens-W]

Let's also add a testcase where !StringUtils.isEmpty(s); is used

[timtebeek]

Whenever you link to the Spring docs, please us a versioned doc link, so containing say /3.3.0 as opposed to /current, as those links can go bad when a next version comes out.

[DidierLoiseau]

Just passing by, but the new implementation will only deal with String compile-time arguments, right?

I think it makes sense to be conservative indeed, and leave the deprecated call if it can’t be guaranteed to keep the original intention.

However, the case of String compile-time argument was actually the only case where using ObjectUtils.isEmpty() would have preserved the semantics, so maybe it’s better than !StringUtils.hasLength()? I think the former remains more readable.

Since ObjectUtils.isEmpty() can’t be used, I wonder if there is any choice that can’t be considered as too opinionated though.

[dimijdriven]

Just passing by, but the new implementation will only deal with String compile-time arguments, right?

I think it makes sense to be conservative indeed, and leave the deprecated call if it can’t be guaranteed to keep the original intention.

However, the case of String compile-time argument was actually the only case where using ObjectUtils.isEmpty() would have preserved the semantics, so maybe it’s better than !StringUtils.hasLength()? I think the former remains more readable.

Since ObjectUtils.isEmpty() can’t be used, I wonder if there is any choice that can’t be considered as too opinionated though.

I think you are right, also I realized after your comment that probably my PR does not solve the issue, on the contrary it will cause compile errors for this specific user.

But it is a weird case all around, an object was accepted for a String comparison, probably that is why they deprecated this method and the user was putting as input for an object a list!

I replicated and a minimum example would be the following:
ObjectUtils.isEmpty(Collections.EMPTY_LIST); // returns true
StringUtils.isEmpty(Collections.EMPTY_LIST); // returns false

So not sure if it actually makes sense to fix that case.

[dimijdriven]

Closed after the comment of @DidierLoiseau

[DidierLoiseau]

But it is a weird case all around, an object was accepted for a String comparison, probably that is why they deprecated this method and the user was putting as input for an object a list!

Yes, I would assume so too. If you check the StringUtils.isEmpty() implementation it is str == null || "".equals(str). I suppose the goal of having an Object parameter was to avoid checking if some value was actually a String upfront.

I think you are right, also I realized after your comment that probably my PR does not solve the issue, on the contrary it will cause compile errors for this specific user.

Do you think so? As this PR currently stands, it would only do a change when the parameter is actually a String, right? So what could be the issue?

Maybe this PR should just remove the migration entirely instead, since it is unsafe?

[dimijdriven]

But it is a weird case all around, an object was accepted for a String comparison, probably that is why they deprecated this method and the user was putting as input for an object a list!

Yes, I would assume so too. If you check the StringUtils.isEmpty() implementation it is str == null || "".equals(str). I suppose the goal of having an Object parameter was to avoid checking if some value was actually a String upfront.

I think you are right, also I realized after your comment that probably my PR does not solve the issue, on the contrary it will cause compile errors for this specific user.

Do you think so? As this PR currently stands, it would only do a change when the parameter is actually a String, right? So what could be the issue?
The person who opened the ticket, passes a list to the function and that works because it accepts objects. The current migration accepts also objects so it compiles, just does not return the correct result. If it is refactored to what I did it will only accept String as parameters and I expect that this will cause a compilation error because List is not String.

Maybe this PR should just remove the migration entirely instead, since it is unsafe?
This would solve the issue, but not the deprecation. I would not be sure if the ideal solution is to remove the migration or just accept that the user was an extreme edge case and accept that in this case the migration breaks.

[DidierLoiseau]

The person who opened the ticket, passes a list to the function and that works because it accepts objects. The current migration accepts also objects so it compiles, just does not return the correct result. If it is refactored to what I did it will only accept String as parameters and I expect that this will cause a compilation error because List is not String.

No but with this PR, it would only do the change when the compile-time type of the expression used as argument for StringUtils.isEmpty() is a String, if my understanding of Refaster is correct.

This would solve the issue, but not the deprecation. I would not be sure if the ideal solution is to remove the migration or just accept that the user was an extreme edge case and accept that in this case the migration breaks.

I don’t think OpenRewrite recipes should be expected to fix all deprecation issues.

[Laurens-W]

The person who opened the ticket, passes a list to the function and that works because it accepts objects. The current migration accepts also objects so it compiles, just does not return the correct result. If it is refactored to what I did it will only accept String as parameters and I expect that this will cause a compilation error because List is not String.

No but with this PR, it would only do the change when the compile-time type of the expression used as argument for StringUtils.isEmpty() is a String, if my understanding of Refaster is correct.

This would solve the issue, but not the deprecation. I would not be sure if the ideal solution is to remove the migration or just accept that the user was an extreme edge case and accept that in this case the migration breaks.

I don’t think OpenRewrite recipes should be expected to fix all deprecation issues.

This is correct, and even if refaster doesn't cut it you could make the recipe "strongly typed" so to say