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

Added maximum string length constraint. (#449) #452

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lucascz37
Copy link

Adds maxStringLengthConstraint field to personalize the readStreamConstraints of JacksonCore. By default the maxStringLength of the StreamReadConstraint is 20_000_000, from what I understood if the json files surpass that size it will throw the error described in issue #449 .

Testing done

I executed three builds with .json files from my tests.

  1. Setting the maxStringLengthConstraint to 25_000_000.
  2. Default plugin execution leaving the maxStringLengthConstraint default value of 20_000_000.
  3. Limiting the maxStringLengthConstraint to 20. (Forcing to throw the same error as not a valid Cucumber report! String length (20051119) exceeds the maximum length (20000000) #449 )

Relevant links:
https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.15.2
FasterXML/jackson-core#1014

Tests screenshots:
Test 1:
Test-1
Test-1 (2)

Test 2:
Test-2 (2)
Test-2

Test 3:
Test-3 (2)
Test-3

Submitter checklist

Edit tasklist title
Beta Give feedback Tasklist Submitter checklist, more options

Delete tasklist

Delete tasklist block?
Are you sure? All relationships in this tasklist will be removed.
  1. Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
    Options
  2. Ensure that the pull request title represents the desired changelog entry
    Options
  3. Please describe what you did
    Options
  4. Link to relevant issues in GitHub or Jira
    Options
  5. Link to relevant pull requests, esp. upstream and downstream changes
    Options
  6. Ensure you have provided tests - that demonstrates feature works or fixes the issue
    Options

@lucascz37 lucascz37 requested a review from a team as a code owner December 6, 2023 18:38
@@ -78,6 +79,7 @@ public class CucumberReportPublisher extends Recorder implements SimpleBuildStep
private boolean undefinedAsNotFailingStatus;

private int trendsLimit;
private int maxStringLengthConstraint = 20_000_000;
Copy link
Member

Choose a reason for hiding this comment

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

nice,
would be perfect having reference to the code from that value was taken

Copy link
Author

Choose a reason for hiding this comment

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

@@ -559,6 +570,8 @@ private void generateReport(Run<?, ?> build, FilePath workspace, TaskListener li

setFailingStatuses(configuration);

StreamReadConstraints.overrideDefaultStreamReadConstraints(StreamReadConstraints.builder().maxStringLength(maxStringLengthConstraint).build());
Copy link
Member

Choose a reason for hiding this comment

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

that should be implemented in core component https://github.com/damianszczepanik/cucumber-reporting

Copy link
Author

Choose a reason for hiding this comment

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

On it.

@damianszczepanik
Copy link
Member

@lucascz37 ping

@damianszczepanik
Copy link
Member

Are you @lucascz37 going to work on this ?

@lucascz37
Copy link
Author

Hi @damianszczepanik sorry for the delay, I was on vacation, I will work on this in the evening today

<f:entry
title="${%maxStringLengthConstraint.title}"
field="maxStringLengthConstraint">
<f:textbox default="20000000"/>
Copy link

@felipecrs felipecrs Feb 21, 2024

Choose a reason for hiding this comment

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

Perhaps we should consider bumping it a little by default. Maybe 25M? This would cover the needs of all the people at #449.

What do you think, @damianszczepanik?

In such case, maybe making it configurable isn't even required (unless someone makes a case where a higher constraint is needed).

Copy link

Choose a reason for hiding this comment

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

@damianszczepanik , would you consider this simple fix from @felipecrs for #449?

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

Successfully merging this pull request may close these issues.

None yet

4 participants