Skip to content

Conversation

asolntsev
Copy link
Collaborator

@asolntsev asolntsev commented Sep 16, 2025

... to return a valid string of length 2..64 instead of empty string.

NB! This PR contains a breaking change:

    lorem.characters(-1) // before: returned "", now: throws exception
    lorem.characters(-1, true) // before: returned "", now: throws exception
    lorem.characters(-1, true, true, true) // before: returned "", now: throws exception

@asolntsev asolntsev added the bug Something isn't working label Sep 16, 2025
@asolntsev asolntsev self-assigned this Sep 16, 2025
@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.42%. Comparing base (2923e4a) to head (7558ba1).

Files with missing lines Patch % Lines
...c/main/java/net/datafaker/providers/base/Text.java 80.00% 1 Missing and 2 partials ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1660      +/-   ##
============================================
- Coverage     92.45%   92.42%   -0.04%     
- Complexity     3388     3393       +5     
============================================
  Files           333      333              
  Lines          6697     6706       +9     
  Branches        665      667       +2     
============================================
+ Hits           6192     6198       +6     
- Misses          346      347       +1     
- Partials        159      161       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@asolntsev asolntsev added this to the 2.5.1 milestone Sep 16, 2025
@asolntsev asolntsev requested review from snuyanzin, kingthorin and bodiam and removed request for snuyanzin and kingthorin September 16, 2025 20:26
public String text(int minimumLength, int maximumLength, boolean includeUppercase, boolean includeSpecial, boolean includeDigit) {
public String text(int requestedMinimumLength, int maximumLength, boolean includeUppercase, boolean includeSpecial, boolean includeDigit) {
if (requestedMinimumLength > maximumLength) {
throw new IllegalArgumentException("Min length (%s) should be not greater than max length (%s)".formatted(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"...should not be..."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, fixed.


final int minimumLength = Math.max(requestedMinimumLength, min(includeUppercase) + min(includeSpecial) + min(includeDigit));
if (minimumLength > maximumLength) {
throw new IllegalArgumentException("Minimum number of required characters (%s) should be not greater than max length (%s)".formatted(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"...should not be..."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

@asolntsev asolntsev force-pushed the fix/1659-min-text-length branch from 747df61 to 4c4f62c Compare September 16, 2025 21:06
Copy link
Collaborator

@kingthorin kingthorin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@erikpragt-connectid
Copy link

erikpragt-connectid commented Sep 17, 2025

I'm not sure about this change. If we implement this, suddenly uppercase/lowercase becomes more important than the String length, since when you say: 1..64, it will never be one.

Also, this change would be a breaking change without people knowing it. I'm not a huge fan.

Would it be possible to have some validation logic on it? Like if you do:

faker.text().text(1, 64, true, false, true)

It will throw an exception, since this configuration can never work? (note, this would also be a breaking runtime change without clear communication to our users), some code might suddenly start failing.

Perhaps we should keep it as in, and instead make a (validating) text builder or so? faker.text().minLength(1).maxLength(60).withUppercase().withLowercase()

@asolntsev
Copy link
Collaborator Author

some code might suddenly start failing

@erikpragt-connectid
No, nothing will fail. Let me clearly emphasize: the line faker.text().text(1, 64, true, false, true) did not fail, and will not fail.
What actually will change is:

  1. BEFORE: sometimes it returned an empty string - which clearly breaks the requirement "from 1 to 64".
  2. AFTER: it will always return a valid string of length 2...64 - which is totally valid response to the requirement "from 1 to 64".

@asolntsev asolntsev force-pushed the fix/1659-min-text-length branch from 91cb744 to 057fa44 Compare September 17, 2025 18:38
@asolntsev
Copy link
Collaborator Author

@erikpragt-connectid Added javadoc:

     * @param minimumLength The generated text will not be shorter than this length
     * @param maximumLength The generated text will not be longer than this length

@bodiam
Copy link
Contributor

bodiam commented Sep 19, 2025

No, nothing will fail.

How so? The behaviour of the method changed, and consumers might be depending on this behaviour in their code. Why do you think they won't fail?

In my understanding:

faker.text().text(1, 64, true, false, false)
faker.text().text(1, 64, true, true, false)
faker.text().text(1, 64, true, true, true)

Should all return a String of 1..64, wouldn't that make a lot of sense?
Not 1..64, 2..64, 3..64, just because you might also allow numbers and lower case.
I would assume that the flags indicate that the string MAY contain one of the characters, not that it MUST contain.

I wouldn't know what this would do though:

faker.text().text(1, 64, false, false, false)

@asolntsev
Copy link
Collaborator Author

@bodiam No, this method works differently. :)

  1. faker.text().text(1, 64, false, false, false) returns 1..64, all lower case Latin letters
  2. faker.text().text(1, 64, true, false, false) returns 1..64, lower and upper case; at least one upper guaranteed.
  3. faker.text().text(1, 64, false, false, true) returns 1..64, lower case and digits; at least one digit guaranteed.
  4. etc.

This is the logic usually needed for password generation.

* @param includeSpecial is at least one special symbol required
* @param includeDigit is at least one digit required
* @return a random string withing given length interval
* @see Text#DEFAULT_SPECIAL
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems need to add a comment that it could throw exception

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added @throws javadoc.

... to return a valid string of length 2..64 instead of empty string.
We just don't need to spend so much energy. Let's keep the nature. :)
@asolntsev asolntsev force-pushed the fix/1659-min-text-length branch from 057fa44 to 7558ba1 Compare September 23, 2025 19:20
@asolntsev asolntsev requested a review from snuyanzin September 23, 2025 19:21
@asolntsev asolntsev merged commit e83d53c into main Sep 23, 2025
13 checks passed
@asolntsev asolntsev deleted the fix/1659-min-text-length branch September 23, 2025 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Faker().text().text(1, 64, true, false, true) returns empty String when length is 1
6 participants