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

Revisit Assert to avoid single-arg assert methods (with refined messages) [SPR-15196] #19761

Closed
spring-issuemaster opened this issue Jan 27, 2017 · 10 comments

Comments

@spring-issuemaster
Copy link
Collaborator

commented Jan 27, 2017

Stéphane Nicoll opened SPR-15196 and commented

See #1307


Issue Links:

  • DATACASS-394 Fix NoSuchMethodError: org.springframework.util.Assert.notNull(Ljava/lang/Object;)V
  • DATACMNS-985 Remove references to single-argument assertion methods of Spring
  • DATACOUCH-274 Remove references to single-argument assertion methods of Spring
  • DATAES-329 Remove references to single-argument assertion methods of Spring
  • DATAGRAPH-966 Remove references to single-argument assertion methods of Spring.
  • DATAJPA-1054 Remove references to single-argument assertion methods of Spring
  • DATAMONGO-1602 Remove references to single-argument assertion methods of Spring
  • DATAREST-992 Remove references to single-argument assertion methods of Spring
  • DATASOLR-361 Remove references to single-argument assertion methods.
  • #19020 Introduce Supplier message support in org.springframework.util.Assert
  • DATACASS-393 Remove references to Assert single-arg methods
  • AMQP-705 NoSuchMethodError: Assert.notNull(Object)
  • DATAREDIS-599 Remove references to single-argument assertion methods of Spring.
  • INT-4221 Spring Framework doesn't suppport single-arg methods in Assert any more

1 votes, 7 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 27, 2017

Ruben Dijkstra commented

This is indeed an issue I've sometimes fixed across a few Spring projects.
spring-projects/spring-data-cassandra#77
spring-projects/spring-security#3905

I think it's good to finally tackle this at the source, but never had a good idea on how to do it well.
With static analysis, you could say that no constants/literals should be passed in, but don't know what the impact would be for that.

(just pitching in since I'm the original issue reporter, sort of)

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 27, 2017

Juergen Hoeller commented

I considered deprecating the single-arg methods in Assert before, leaving just the two-arg variants with an explicit message argument... so people wouldn't accidentally call a single-arg variant with a plain message anymore. In 5.0, we can even remove the single-arg variants completely.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 28, 2017

Sam Brannen commented

I considered deprecating the single-arg methods in Assert before, leaving just the two-arg variants with an explicit message argument... so people wouldn't accidentally call a single-arg variant with a plain message anymore. In 5.0, we can even remove the single-arg variants completely.

(y)

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 29, 2017

Juergen Hoeller commented

The single-arg methods are gone in 5.0 now, which nicely goes with the new Supplier variants introduced in #19020, bringing the number of methods per condition to just two again.

Correspondingly, in 4.3.x, I'll simply deprecate the single-arg methods.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2017

Ruben Dijkstra commented

This is good news, thanks for picking this up!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2017

Dmitry Bakaleinik commented

Hi all,
Unfortunately this change broke projects that uses spring-boot 2.0 devel snapshots.
So Juergen Hoeller and Sam Branner were right about deprecating method before fully removal.

As a developer I know that the easiest way to migrate all code to new api - break something and then fix dependent projects one by one.
But please, let to people adapt their code, don't break things in one moment. Better to split such major change to phases - release new api with deprecation of old method, then remove it only when other spring projects be fully ready.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 31, 2017

Juergen Hoeller commented

I'm impressed! Our feedback loop works great, and wow, people are actually using Boot 2.0 snapshots already :-)

Just to be clear, this was a change in a pre-milestone snapshot only. I intentionally removed those methods to raise awareness, making our portfolio projects notice those Assert calls and include a proper exception message there, since deprecation is too easy to ignore. The obvious fix is to use the variants with a String exception message which have been around for a decade, so remain compatible with 4.x.

I do want to get rid of those single-arg methods for 5.1 at least, avoiding accidental use and reducing the number of overloaded methods to two again (one with a String message - the common option that's been in around for many years - and our new Supplier<String> variant). That said, I've just added them back to master in deprecated form, with the same deprecation markers that they'll have in the upcoming 4.3.7 release.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 13, 2017

Andy Wilkinson commented

The change to Assert.isInstanceOf(Class, Object, String) means that its javadoc is now misleading. The javadoc says that message should "normally end in ":" or "." so that the generated message looks OK when appended to it". That doesn't appear to be good advice any more and leads to a double : in the message. For example, this code:

Assert.isInstanceOf(java.lang.Long.class, new java.lang.Integer(0), "The ID must be a Long: ");

Results in this message:

The ID must be a Long: : java.lang.Integer

I'd like to be able to have a message that says something like "ID must be an instance of java.lang.Long but its type was java.lang.Integer". If the behavior matched the javadoc I think I'd be able to do this.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 16, 2017

Juergen Hoeller commented

Actually, your case wouldn't have worked with the 'old' Assert either: We always appended a full exception message there, no matter what the separator at the end of your message was.

So I've revised this for 4.3.7 towards a bit more sophistication now: If the given message ends with a separator (colon, semicolon, comma, period), we append the full exception message as we did in 4.3.6. If the given message ends with a space, we just append the offending type name (making your case work). If neither of the above is the case, we append ": " and the offending type name (keeping my common case working for the revised assertion statements in 5.0 / 4.3.7).

I'll backport this to 4.3.7 ASAP.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 16, 2017

Juergen Hoeller commented

Available in the latest 4.3.7.BUILD-SNAPSHOT now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.