Assertj-core: null is not blank?!

[eximius313]

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]

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.

[joel-costigliola]

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

[eximius313]

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

[eximius313]

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

[joel-costigliola]

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

[jbspeakr]

👍

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

[ChrisCanCompute]

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

[jbspeakr]

@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.

[joel-costigliola]

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!

[PascalSchumacher]

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).

[joel-costigliola]

@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 :)

[PascalSchumacher]

@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).

[eximius313]

+1 for containsOnlyWhitespace naming

[joel-costigliola]

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

[PascalSchumacher]

+1

[ChrisCanCompute]

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?

[ChrisCanCompute]

Attempt number 2. Let me know what you think.