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-21017] When unmarshalling into an existing object, reset missing fields #2736

Merged
merged 2 commits into from Dec 29, 2017

Conversation

6 participants
@stephenc
Copy link
Member

stephenc commented Feb 6, 2017

See JENKINS-21017

@jenkinsci/code-reviewers @reviewbybees

Notes:

  • Likely we will need a good bit of testing before we consider merging
  • Needs at least a full clean test run and a full ATH run before I am ready to claim this is even close to mergable
@jglick

jglick approved these changes Feb 6, 2017

Copy link
Member

jglick left a comment

Seems right. I think the key advance here was in finding an existing general-purpose Converter defined in Jenkins sources whose behavior could be patched easily.

Would be advisable to also write an integration test for one of the originally reported systems, such as trying to unset an AbstractProject.assignedNode by POSTing to its config.xml.

@@ -371,6 +442,12 @@ public Object doUnmarshal(final Object result, final HierarchicalStreamReader re
OldDataMonitor.report((Saveable)result, (ArrayList<Throwable>)context.get("ReadError"));
context.put("ReadError", null);
}
if (existingObject) {
for (Map.Entry<FieldExpectation, Object> entry : expectedFields.entrySet()) {
reflectionProvider.writeField(result, entry.getKey().getName(), entry.getValue(),

This comment has been minimized.

Copy link
@jglick

jglick Feb 6, 2017

Member

Does this wind up writing non-null fields of root objects twice?

This comment has been minimized.

Copy link
@stephenc

stephenc Feb 6, 2017

Author Member

No, these are the fields that were not overwritten, and this is only for an already existing object.

I think the issue also affects non root objects only people have got away without noticing because of the root object blocker (or else the child objects are not updated)... need tests to verify further

This comment has been minimized.

Copy link
@abayer

abayer Jun 16, 2017

Member

Actually, it looks to me like it does - the expected field value is always uninitialized (see above discussion), and it's not checking whether the field was already seen, so it's overwriting with null/0/false/etc.

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

LGTM though I have not tested it

assertThat("Deserializing over an new instance is the same as with no root", xmlB, is(xmlC));
}

public static class WithDefaults {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 6, 2017

Member

Builder pttern would be preferable

public Object doUnmarshal(final Object result, final HierarchicalStreamReader reader, final UnmarshallingContext context) {
final SeenFields seenFields = new SeenFields();
final boolean existingObject = context.currentObject() != null;
final Map<FieldExpectation, Object> expectedFields = existingObject ? new HashMap<FieldExpectation, Object>() : null;
final Object cleanInstance = existingObject ? reflectionProvider.newInstance(result.getClass()) : null;

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Feb 7, 2017

Member

This and the preceding line would be clearer if they just initialized to null and were populated in the if block.

This comment has been minimized.

Copy link
@daspilker

daspilker Feb 9, 2017

Member

reflectionProvider always seems to be an instance of SunLimitedUnsafeReflectionProvider which uses sun.misc.Unsafe#allocateInstance to create a new instance. AFAIK Unsafe#allocateInstance does not call any constructor, so fields will not be initialized. So the value of each field in cleanInstance will be null, 0, false, etc. That's why there are so many NPEs in the tests.

This comment has been minimized.

Copy link
@abayer

This comment has been minimized.

Copy link
@abayer

abayer Jun 16, 2017

Member

Clarification - the init NPEs go away when I force it to bypass anything involving clouds or views, which conveniently appear to be the only final non-transient fields initialized in Jenkins. Coincidence? Mayhaps, I dunno.

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Feb 7, 2017

A test using POST config.xml and a few top-level elements e.g. in FreestyleJob would be useful, e.g. confirming that description would be unset, or that all build steps are removed as expected.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Feb 7, 2017

I think the issue also affects non root objects

I doubt it. Everything not at top level is just constructed from scratch. There is no XStream equivalent of ReconfigurableDescribable that I know of.

@daspilker

This comment has been minimized.

Copy link
Member

daspilker commented Feb 9, 2017

👍

I tested the examples from JENKINS-26825, JENKINS-30548 and JENKINS-39917 with Job DSL. All work as expected, so this PR seems to fix the problem.

@stephenc

This comment has been minimized.

Copy link
Member Author

stephenc commented Feb 9, 2017

@daspilker yes but I now have a lot of NPEs in the test cases deserialising other objects, so I need to investigate what exactly is going on there

stephenc added a commit to stephenc/scm-api-plugin that referenced this pull request Jun 14, 2017

[JENKINS-43507] Bobby didn't believe the test was real
(the object instance itself is returned, but with all its field
instances recreated... something that I was trying to fix in
jenkinsci/jenkins#2736 but I hit issues.
I am switching to the more explicit comparison as future-proofing
for when the underlying issue in jenkins-core#2736 is fixed in
order to ensure that the test remains valid)
@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jun 16, 2017

@stephenc Could you merge from master to this to resolve the conflict?

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jun 16, 2017

So I'm trying to figure out the test failures, and the most definite problem is that at least two fields in Jenkins (clouds and views) are ending up with null values rather than what they're initialized with. These two fields are both final and not transient and seem to be the only fields with that combination of modifiers. I'm also concerned that the expectedFields fields are getting overwritten even if they're already set, but not sure on that yet.

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jun 16, 2017

Verified on the overwriting - the expectedFields.remove(...) calls aren't working, because the classDefiningField seems to always be null (at least with, e.g., the ClassicPluginStrategyTest tests) and result.getClass() is hudson.model.Hudson but the FieldExpectation.definingClass is being set to, say, jenkins.model.Jenkins, hudson.model.Node, hudson.model.AbstractCIBase, etc. So fields like jdk are getting written once with the right value and then overwritten with null, etc.

So basically, this is all busted in new and interesting ways.

@abayer
Copy link
Member

abayer left a comment

👎 until this actually works as intended. Which it does not.

abayer added a commit to abayer/jenkins that referenced this pull request Jun 16, 2017

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jun 16, 2017

@stephenc I do have an experiment at abayer@e27e0f2 that seems to get past at least some of the problems, but I'm not sure if it's just terrible on the face of it. So I defer to you.

EDIT: Looks like either my change or the original is breaking @LocalData. Not sure which, since without my change it can't even get to the point of doing anything with @LocalData. shrug

EDIT 2: Clarification - @LocalData seems to work many places but not JobTest at a minimum. And ViewTest is well and properly borked by the original change.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Aug 1, 2017

It would seem to suffice for the unmarshal(HierarchicalStreamReader, Object) method to just look for all declared fields in the root object and null them out before calling the regular implementation. Or am I missing something?

@oleg-nenashev oleg-nenashev merged commit 31a2bc1 into jenkinsci:master Dec 29, 2017

1 check failed

continuous-integration/jenkins/pr-head The build of this commit was aborted
Details
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.