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

null is not blank?! #1069

Closed
eximius313 opened this issue Sep 15, 2017 · 17 comments
Closed

null is not blank?! #1069

eximius313 opened this issue Sep 15, 2017 · 17 comments

Comments

@eximius313
Copy link

@eximius313 eximius313 commented Sep 15, 2017

Summary

As far as I know, null is considered as "Blank" (as "Blank" is superset of null, empty string or blank characters only)

Example

This passes:

        String s = null;
        assertThat(s).isNotBlank();

Java 8 specific ?

  • NO : create Pull Request from the 2.x branch
@joel-costigliola
Copy link
Member

@joel-costigliola joel-costigliola commented Sep 15, 2017

It is the behavior as stated in the javadoc: http://joel-costigliola.github.io/assertj/core/api/org/assertj/core/api/AbstractCharSequenceAssert.html#isBlank().

Whether that should be changed is another story as it would be a breaking change.
I would be ok to change it though if there is a clear definition of blank.

Loading

@joel-costigliola
Copy link
Member

@joel-costigliola joel-costigliola commented Sep 16, 2017

One option would to rename isBlank to isStrictlyBlankand add isBlank that would accept the Apache commons definition of blank.

Loading

@eximius313
Copy link
Author

@eximius313 eximius313 commented Sep 16, 2017

sounds reasonable, as this "blank" definition is quite common and a lot of projects are using StringUtils

Loading

@eximius313
Copy link
Author

@eximius313 eximius313 commented Sep 16, 2017

Wait a second! The documentation you mentioned clearly states:

Whereas these assertions will fail:
[cut]
String nullString = null;
assertThat(nullString).isNotBlank();

While it passes! So my issue is a bug and there is no need for isStrictlyBlank

Loading

@joel-costigliola
Copy link
Member

@joel-costigliola joel-costigliola commented Sep 16, 2017

The bug is actually in the isBlank javadoc assertThat(nullString).isNotBlank(); should be replaced by assertThat(nullString).isBlank();, the isNotBlank javadoc is correct.

Loading

@jbspeakr
Copy link

@jbspeakr jbspeakr commented Oct 23, 2017

👍

just also stumbled upon this misbehaviour and would like to see this fixed and consistent with Apache Commons StringUtils and Hibernate Validators!

Loading

ChrisCanCompute pushed a commit to ChrisCanCompute/assertj-core that referenced this issue Oct 23, 2017
@ChrisCanCompute
Copy link
Contributor

@ChrisCanCompute ChrisCanCompute commented Oct 23, 2017

I'm going to add both isStrictlyBlank() and isNotStrictlyBlank() for completeness :)

Loading

ChrisCanCompute pushed a commit to ChrisCanCompute/assertj-core that referenced this issue Oct 23, 2017
ChrisCanCompute pushed a commit to ChrisCanCompute/assertj-core that referenced this issue Oct 23, 2017
ChrisCanCompute added a commit to ChrisCanCompute/assertj-core that referenced this issue Oct 23, 2017
ChrisCanCompute added a commit to ChrisCanCompute/assertj-core that referenced this issue Oct 23, 2017
ChrisCanCompute added a commit to ChrisCanCompute/assertj-core that referenced this issue Oct 23, 2017
ChrisCanCompute added a commit to ChrisCanCompute/assertj-core that referenced this issue Oct 23, 2017
@joel-costigliola joel-costigliola added this to the 2.9.0 milestone Oct 23, 2017
@jbspeakr
Copy link

@jbspeakr jbspeakr commented Oct 24, 2017

@ChrisCanCompute sorry for being slightly opinionated about this, but I don't think this is about completeness but correctness.

As @eximius313 pointed out, there is already a widespread and commonly shared understanding of what Blank actually means. I do think that AssertJ having a different definition is actually a bug.

It's not about having yet another method for a simple check. It's about being able to use a well established domain language also in AssertJ.

Loading

@joel-costigliola
Copy link
Member

@joel-costigliola joel-costigliola commented Oct 24, 2017

At this point I think I can say we (that is me at 98.75%) have screwed up the blank related assertions:

  • we have guava blank (isBlank) vs java blank (isJavaBlank) which makes me sad
  • our blank assertion fails for null String, as pointed out it's a common implementation which makes it a de facto standard (I personally still think that null is not blank but I'm fine with giving in to the majority).

From these points, I'm inclined to:

  1. change isBlank to accept null
  2. change isNotBlank to fail on null
  3. change isBlank to use Java definition of whitespace as apache commons-lang
  4. deprecate isJavaBlank and isNotJavaBlank in favor of isBlank and isNotBlank the rationale being that AssertJ should follow Java behavior.
  5. not add isStrictlyBlank as it can be replaced by: isNotNull().isBlank() which has the advantage of being obvious
  6. maybe add isNullOrNotBlank? or just wait for someone to ask for it ?

@PascalSchumacher I'd like to have your opinion before taking a decision on this, thoughts?

@ChrisCanCompute if we go with 5. then it means we have to ignore part of your PR - sorry about that!

Loading

@PascalSchumacher
Copy link
Member

@PascalSchumacher PascalSchumacher commented Oct 24, 2017

As eximius313 said there are more differences, so let's take a detailed look at the behavior of commons-lang compared to assertj:

call commons-lang assertj
isBlank(null) true false
isBlank("") true false
isBlank(non-breaking-whitespace-character) false true
isBlank(breaking-whitespace-character) true true
isBlank(non-whitespace-character) false false

I think the root cause of this is that the assertj definition of an empty string is different from e.g. commons-lang.

assertThat((String) null).isEmpty() fails while StringUtils.isEmpty((String) null) returns true.

If we change isBlank to accept null if we should also change isEmpty to accept null. Otherwise the behavior of the library seems inconsistent.

If I would start a greenfield assertion library I would take the isEmpty and isBlank definitions from commons-lang and combine them with an up-to-date whitespace definition (as currently used by assertj).

Loading

@joel-costigliola
Copy link
Member

@joel-costigliola joel-costigliola commented Oct 25, 2017

@PascalSchumacher good points.

Since there is no clear solution to this, I think the reasonable way to go is to:

  1. change isBlank and isNotBlank to behave like commons lang
  2. forget about Guava blank, users can write a simple condition to have it.
  3. deprecate isJavaBlank and isNotJavaBlank in favor of isBlank and isNotBlank the rationale being that AssertJ should follow Java behavior.
  4. add isStrictlyBlank that keeps the isBlank old behavior, same for isNotStrictlyBlank
  5. in isBlank and isNotBlank javadoc explain the change and point to isStrictlyBlank for users to keep the old behavior.

I will leave isEmpty as it is for the time being.

comments welcome :)

Loading

@PascalSchumacher
Copy link
Member

@PascalSchumacher PascalSchumacher commented Oct 25, 2017

@joel-costigliola I think the by far most common use-case is isNotBlank and use it to check that a string is not null, empty or whitespace only (as reported by eximius313). So if we make isNotBlank fail for null, it should also fail for an empty string.

I'm not a great fan of isStricklyBlank. Imho containsOnlyWhitespace would be a more intention revealing name ( it also similar to the existing method containsOnlyDigits), but I doubt somebody needs this assertion (I could be wrong of course ;)). containsNotOnlyWhitespace would be clearer that isNotStrictlyBlank, what do you think?

I'm o.k. with replacing the more correct whitespace definition with the java one, as most users probably do not care either way (but because of this I also do not see a strong reason to change it).

Loading

@eximius313
Copy link
Author

@eximius313 eximius313 commented Oct 25, 2017

+1 for containsOnlyWhitespace naming

Loading

@joel-costigliola
Copy link
Member

@joel-costigliola joel-costigliola commented Oct 25, 2017

I agree with @PascalSchumacher suggestion on isNotBlank and containsOnlyWhitespaces (note the s at the end).
I would rename containsNotOnlyWhitespace to doesNotContainOnlyWhitespaces.

Loading

@PascalSchumacher
Copy link
Member

@PascalSchumacher PascalSchumacher commented Oct 26, 2017

+1

Loading

@ChrisCanCompute
Copy link
Contributor

@ChrisCanCompute ChrisCanCompute commented Oct 28, 2017

Ok. It looks like the spec is cleared up now.
I'll abandon my pull requests and fix this tomorrow unless anyone has any further comments?

Loading

@ChrisCanCompute
Copy link
Contributor

@ChrisCanCompute ChrisCanCompute commented Oct 31, 2017

Attempt number 2. Let me know what you think.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants