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

[JENKINS-49070] Avoid serializing BigDecimal and BigInteger in the model #239

Merged
merged 5 commits into from Jan 29, 2018

Conversation

@abayer
Copy link
Member

commented Jan 22, 2018

  • JENKINS issue(s):
  • Description:
    • Groovy parses 0.1 into a BigDecimal. So let's convert that to a Double instead in ModelASTValue#value. And also, just barf out if for some reason someone tries to use a BigInteger constant. Because no.
  • Documentation changes:
    • n/a
  • Users/aliases to notify:

@abayer abayer requested review from jglick, rsandell and oleg-nenashev Jan 22, 2018

@oleg-nenashev
Copy link
Member

left a comment

🐝

val = val.doubleValue()
} else if (val instanceof BigInteger) {
if (val > Long.MAX_VALUE || val < Long.MIN_VALUE) {
errorCollector.error(ModelASTValue.fromConstant(-1, e), Messages.ModelParser_BigIntegerValue())

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jan 22, 2018

Member

Maybe converting to Double? The current impl is fine for me though

This comment has been minimized.

Copy link
@abayer

abayer Jan 24, 2018

Author Member

Nah, I want to stick with int-family rather than floating point. Anyway, tweaked in latest commit.


Object realVal = val.getValue();

assertTrue(realVal instanceof Double);

This comment has been minimized.

Copy link
@jglick

jglick Jan 22, 2018

Member

redundant given test below

This comment has been minimized.

Copy link
@abayer

abayer Jan 24, 2018

Author Member

Touché.

if (val > Long.MAX_VALUE || val < Long.MIN_VALUE) {
errorCollector.error(ModelASTValue.fromConstant(-1, e), Messages.ModelParser_BigIntegerValue())
}
val = -1

This comment has been minimized.

Copy link
@jglick

jglick Jan 22, 2018

Member

Uh, I think this will convert, say, new BigInteger(42) to new Integer(-1) which is not what you meant to do. Looks like you meant to put this line inside the above }? Best to add a test confirming behavior of small integers.

This comment has been minimized.

Copy link
@abayer

abayer Jan 24, 2018

Author Member

Whoops, yup. That -1 (when it's put in the right place!) ends up being ignored in practice, because the error trumps it, but yeah, it is in the wrong place.

This comment has been minimized.

Copy link
@abayer

abayer Jan 24, 2018

Author Member

Oh, and this situation can only, in practice, be hit if you specify a constant number greater than Long.MAX_VALUE - we're only handling ConstantExpressions here, so, say, 24 ends up as functionally equivalent to new Integer(24), while 2147483648 (i.e., one more than Integer.MAX_VALUE) will be equivalent to new Long(2147483648). Groovy handles that behind the scenes.

@abayer abayer added this to the 1.2.7 milestone Jan 24, 2018

@abayer abayer requested a review from jglick Jan 24, 2018

@jglick
jglick approved these changes Jan 25, 2018
Copy link
Member

left a comment

By the way, assuming the new test indeed reproduces the problem when run against JEP-200, I would suggest modifying your Jenkinsfile to

-        NEWER_CORE_VERSION = "2.89.2"
+        NEWER_CORE_VERSION = "2.103"
@abayer

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2018

Good idea. Lemme see how that goes.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2018

Ok, this is weird. I think it might be an environmental issue with Docker on the agents. I'll revisit this tomorrow or the weekend.

@abayer abayer force-pushed the abayer:jenkins-49070 branch from 74d246e to 3e18247 Jan 29, 2018

@abayer abayer merged commit 6fd36ef into jenkinsci:master Jan 29, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.