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

Comments

Projects
None yet
5 participants
@eximius313
Copy link

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

This comment has been minimized.

Copy link
Owner

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.

@joel-costigliola

This comment has been minimized.

Copy link
Owner

commented Sep 16, 2017

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

@eximius313

This comment has been minimized.

Copy link
Author

commented Sep 16, 2017

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

@eximius313

This comment has been minimized.

Copy link
Author

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

@joel-costigliola

This comment has been minimized.

Copy link
Owner

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.

@jbspeakr

This comment has been minimized.

Copy link

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!

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

@ChrisCanCompute

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2017

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

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

This comment has been minimized.

Copy link

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.

@joel-costigliola

This comment has been minimized.

Copy link
Owner

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!

@PascalSchumacher

This comment has been minimized.

Copy link
Collaborator

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

@joel-costigliola

This comment has been minimized.

Copy link
Owner

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

@PascalSchumacher

This comment has been minimized.

Copy link
Collaborator

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

@eximius313

This comment has been minimized.

Copy link
Author

commented Oct 25, 2017

+1 for containsOnlyWhitespace naming

@joel-costigliola

This comment has been minimized.

Copy link
Owner

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.

@PascalSchumacher

This comment has been minimized.

Copy link
Collaborator

commented Oct 26, 2017

+1

@ChrisCanCompute

This comment has been minimized.

Copy link
Contributor

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?

@ChrisCanCompute

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2017

Attempt number 2. Let me know what you think.

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