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

Revert changes to toString() in FieldError #30799

Closed
asjp1970 opened this issue Jul 3, 2023 · 3 comments
Closed

Revert changes to toString() in FieldError #30799

asjp1970 opened this issue Jul 3, 2023 · 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: regression A bug that is also a regression
Milestone

Comments

@asjp1970
Copy link

asjp1970 commented Jul 3, 2023

Affects: 5.3.27 and later versions


In spring-framework 5.3.27 ObjectUtils.nullSafeConciseToString() method was introduced (#30287), that changed the way to generate null-safe, concise string representation of a supplied object.

We are indirectly using that method from spring-context via FieldError.toString(), to retrieve the string representation of a field failing JSR303 bean validation.

When upgrading from 5.3.24 to 5.3.27 we have had contract tests failing because in case of empty lists that should have at least 1 element, failing validation, the value returned for them was not anymore the expected [], but ArrayList@[the_address].

I'll try to summarize:

Imagine you are in FieldError because the Spring validation wrapping stuff has produced a MethodArgumentNotValidException, the exception handler in the application catches it, and it calls FieldError.toString() to fill in a ProblemDetails.

With 5.3.24:

// snippet in the ApplicationResponseExceptionHandler:

private String getFieldErrorStr(final FieldError fieldErr) {
    String result = "";
    if (fieldErr != null) {
      final String fullErr = fieldErr.toString();
      result = fullErr.substring(0, fullErr.indexOf(";"));
    }
    return result;
  }

// calling this in FieldError:
@Override
public String toString() {
  return "Field error in object '" + getObjectName() + "' on field '" + this.field +
    "': rejected value [" + ObjectUtils.nullSafeToString(this.rejectedValue) + "]; " +
	resolvableToString();
}

// ...which in the case of an empty collection ended up here in org.springframework.util.ObjectUtils:
public static String nullSafeToString(@Nullable Object obj) {
  if (obj == null) {
    return NULL_STRING;
  }
  if (obj instanceof String) {
    return (String) obj;
  }
  if (obj instanceof Object[]) {
   return nullSafeToString((Object[]) obj);
  }
  if (obj instanceof boolean[]) {
    return nullSafeToString((boolean[]) obj);
  }
  if (obj instanceof byte[]) {
    return nullSafeToString((byte[]) obj);
  }
  if (obj instanceof char[]) {
    return nullSafeToString((char[]) obj);
  }
  if (obj instanceof double[]) {
    return nullSafeToString((double[]) obj);
  }
  if (obj instanceof float[]) {
    return nullSafeToString((float[]) obj);
  }
  if (obj instanceof int[]) {
    return nullSafeToString((int[]) obj);
  }
  if (obj instanceof long[]) {
    return nullSafeToString((long[]) obj);
  }
  if (obj instanceof short[]) {
    return nullSafeToString((short[]) obj);
  }
  String str = obj.toString();
  return (str != null ? str : EMPTY_STRING);
}

For something like a list implementation, when empty, all the if's failed and the call to obj.toString() before the return statement ended in java.util.AbstractCollection.toString (that is the case of java.util.ArrayList), which produced the expected result:

public String toString() {
  Iterator<E> it = iterator();
  if (! it.hasNext())
    return "[]";
  ...
}

With 5.3.27 (and onwards):

// snippet in the ApplicationResponseExceptionHandler same as before

// calling this in FieldError:
@Override
public String toString() {
  return "Field error in object '" + getObjectName() + "' on field '" + this.field +
    "': rejected value [" + ObjectUtils.nullSafeConciseToString(this.rejectedValue) + "]; " +
	resolvableToString();
}

// ...which in the case of an empty collection ended up here in org.springframework.util.ObjectUtils:
public static String nullSafeConciseToString(@Nullable Object obj) {
   if (obj == null) {
    return "null";
  }
  if (obj instanceof Class<?>) {
    return ((Class<?>) obj).getName();
  }
  if (obj instanceof CharSequence) {
    return StringUtils.truncate((CharSequence) obj);
  }
  Class<?> type = obj.getClass();
  if (isSimpleValueType(type)) {
     String str = obj.toString();
     if (str != null) {
     return StringUtils.truncate(str);
   }
  }
  return type.getTypeName() + "@" + getIdentityHexString(obj);
}

As before, with an empty list, obj does not enter any of the instanceof if's, neither the isSimpleValueType. As a result ArrayList@12345 is returned, which is wrong and not what we expect as the value making the validation fail, which is an empty list and not something like ArrayList@12345.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 3, 2023
@sbrannen sbrannen added type: regression A bug that is also a regression in: core Issues in core modules (aop, beans, core, context, expression) labels Jul 3, 2023
@sbrannen sbrannen changed the title ObjectUtils returns a wrong string representation of an empty collection ("ArrayList@12345" instead of "[]") Provide explicit support for empty collections and arrays in ObjectUtils.nullSafeConciseToString() Jul 3, 2023
@sbrannen sbrannen self-assigned this Jul 3, 2023
@sbrannen sbrannen changed the title Provide explicit support for empty collections and arrays in ObjectUtils.nullSafeConciseToString() Provide explicit support for empty collections/maps/arrays in ObjectUtils.nullSafeConciseToString() Jul 3, 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 type: regression A bug that is also a regression labels Jul 3, 2023
@sbrannen sbrannen added this to the 6.0.11 milestone Jul 3, 2023
@sbrannen sbrannen changed the title Provide explicit support for empty collections/maps/arrays in ObjectUtils.nullSafeConciseToString() Provide explicit support for empty collections/maps/arrays, Optional, File, and Path in ObjectUtils.nullSafeConciseToString() Jul 3, 2023
@sbrannen sbrannen changed the title Provide explicit support for empty collections/maps/arrays, Optional, File, and Path in ObjectUtils.nullSafeConciseToString() Support empty collections/maps/arrays, Optional, File, and Path in ObjectUtils.nullSafeConciseToString() Jul 3, 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 labels Jul 3, 2023
@sbrannen sbrannen changed the title Support empty collections/maps/arrays, Optional, File, and Path in ObjectUtils.nullSafeConciseToString() Support empty collections, maps, and arrays in ObjectUtils.nullSafeConciseToString() Jul 4, 2023
@sbrannen sbrannen changed the title Support empty collections, maps, and arrays in ObjectUtils.nullSafeConciseToString() Provide explicit support collections, maps, and arrays in ObjectUtils.nullSafeConciseToString() Jul 4, 2023
@sbrannen sbrannen changed the title Provide explicit support collections, maps, and arrays in ObjectUtils.nullSafeConciseToString() Provide explicit support for collections, maps, and arrays in ObjectUtils.nullSafeConciseToString() Jul 4, 2023
@sbrannen sbrannen changed the title Provide explicit support for collections, maps, and arrays in ObjectUtils.nullSafeConciseToString() Revert changes to toString() in FieldError Jul 4, 2023
@sbrannen sbrannen added type: regression A bug that is also a regression and removed type: enhancement A general enhancement labels Jul 4, 2023
@sbrannen sbrannen removed their assignment Jul 4, 2023
@sbrannen
Copy link
Member

sbrannen commented Jul 4, 2023

Thank you for raising the issue.

Due to your feedback and feedback from others in the community, we have decided to revert the changes to FieldError#toString in 6.0.11 and 5.3.29.

@asjp1970
Copy link
Author

asjp1970 commented Jul 4, 2023

You're wellcome.
What version of SpringBoot will include 5.3.29 ¿Do you know? Thanks

@jhoeller jhoeller self-assigned this Jul 4, 2023
@jhoeller
Copy link
Contributor

jhoeller commented Jul 4, 2023

Spring Boot 2.7.14 will include this, as well as the other upcoming Spring Boot releases in July.

jhoeller added a commit that referenced this issue Jul 4, 2023
We would preferably use ObjectUtils.nullSafeConciseToString(rejectedValue) here but revert to the full nullSafeToString representation for strict backwards compatibility (programmatic toString calls as well as exception messages).

Closes gh-30799

(cherry picked from commit 1dc9dff)
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: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

4 participants