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

[FIXED JENKINS-32359] - Properly assign persisted list owners when loading folders from disk #32

Merged
merged 8 commits into from Jun 13, 2016

Conversation

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Jan 8, 2016

The issue causes a critical bug in CloudBees Folders API. I suppose it can be reproduced manually with plugin-internal properties if you follow the unit test.
https://issues.jenkins-ci.org/browse/JENKINS-32359

@reviewbybees

@oleg-nenashev oleg-nenashev mentioned this pull request Jan 8, 2016
7 of 7 tasks complete
@rsandell

This comment has been minimized.

Copy link
Member

rsandell commented Jan 8, 2016

🐝

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Jan 8, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@Vlatombe

This comment has been minimized.

Copy link
Member

Vlatombe commented Jan 8, 2016

🐝

@@ -184,6 +184,8 @@ protected AbstractFolder(ItemGroup parent, String name) {
protected void init() {
if (properties == null) {
properties = new DescribableList<AbstractFolderProperty<?>,AbstractFolderPropertyDescriptor>(this);
} else {
properties.setOwner(this);

This comment has been minimized.

Copy link
@jtnord

jtnord Jan 8, 2016

Member

why is this done on init (called from onLoad()) rather than in readResolve?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jan 8, 2016

Author Member

Maybe it's a lazy hack allowing to call the same method from constructors

This comment has been minimized.

Copy link
@jglick

jglick Jan 11, 2016

Member

During readResolve, this will in general be incomplete in various ways. Unsafe to pass an instance to plugin code which might expect to do things like call owner.getParent(), etc. onLoad is called later when everything is wired up.


// We add a stub property to generate the persisted list
// After that we save and reload the config in order to drop PersistedListOwner according to the JENKINS-32359 scenario
folder.addProperty(new FolderCredentialsProvider.FolderCredentialsProperty(new DomainCredentials[0]));

This comment has been minimized.

Copy link
@jtnord

jtnord Jan 8, 2016

Member

what is this first part doing that is important to the test that the second part is not doing?

i.e. why 2 properties and 2 reloads - yet you are only asserting on one property being persisted?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jan 8, 2016

Author Member

I need to save a non-empty properties list to the disk and reload it. It makes the code to bypass the if (properties == null) clause

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jan 8, 2016

Author Member

At the end of the day, that's how I've got into it during jenkinsci/ownership-plugin#44 testing :)

@jtnord

This comment has been minimized.

Copy link
Member

jtnord commented Jan 8, 2016

🐝

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jan 8, 2016

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Jan 8, 2016

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Jan 8, 2016

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Jan 8, 2016

🐝 👍

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jan 8, 2016

Another TC is available here: oleg-nenashev/ownership-plugin@b42f293

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jan 8, 2016

Analogous to #19 IIUC.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jan 8, 2016

@jglick
The title of this PR seemed to be unrelevant, so I didn't even open it :(
BTW this PR also has Unit tests

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jan 8, 2016

@oleg-nenashev well #19 purports to fix ownership of FolderIcon, whereas you are trying to fix ownership of FolderProperty.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jan 11, 2016

@jglick Gentle bump, any chance to get a release on this week? It blocks jenkinsci/ownership-plugin#44

// We add a stub property to generate the persisted list
// After that we save and reload the config in order to drop PersistedListOwner according to the JENKINS-32359 scenario
folder.addProperty(new FolderCredentialsProvider.FolderCredentialsProperty(new DomainCredentials[0]));
folder.doReload();

This comment has been minimized.

Copy link
@jglick

jglick Jan 11, 2016

Member

🐛 This is not convincing. You are retaining the local variable reference to the original Folder object, unlike the real situation where a new instance is constructed.

Rather use Jenkins.reload and call getItemByFullName to find the folder again; or use RestartableJenkinsRule in a new test suite.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jan 14, 2016

Author Member

ok

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jan 11, 2016

you are trying to fix ownership of FolderProperty

I was misled. FolderProperty.owner already was set. You are rather fixing the ownership of the property list, relevant only for resaving the folder config.xml when making changes to that list.

assertThat("Folder has not been found after the reloading", reloadedFolder, notNullValue());
assertThat("Property has not been reloaded, hence it has not been saved properly",
reloadedFolder.getProperties().get(com.cloudbees.hudson.plugins.folder.properties.AuthorizationMatrixProperty.class),
notNullValue());

This comment has been minimized.

Copy link
@jglick

jglick Jan 11, 2016

Member

Would be nice to also verify that the AbstractFolderProperty.owner is set on all properties after reload.

Since that is protected, and not trivially accessed from implementations in this plugin, may be easier to use @TestExtension properties for this purpose.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jan 17, 2016

Author Member

I see nothing bad in adding a public getter to AbstractFolder Property.
BTW it can be reduced to package-internal Restricted method

@oleg-nenashev oleg-nenashev changed the title [FIXED JENKINS-32359] - Properly assign persisted list owners when loading folders from disk [FIXED JENKINS-32359,JENKINS-32487] - Properly assign persisted list owners when loading folders from disk Jan 17, 2016
@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jan 17, 2016

@jglick done

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Feb 9, 2016

@jglick bump

@Issue("JENKINS-32487")
@Test public void shouldAssignPropertyOwnerOnCreationAndReload() throws Exception {
Folder folder = r.jenkins.createProject(Folder.class, "myFolder");
r.jenkins.setAuthorizationStrategy(new ProjectMatrixAuthorizationStrategy());

This comment has been minimized.

Copy link
@jglick

jglick Feb 11, 2016

Member

Clashes with #33.

This comment has been minimized.

Copy link
@jglick

jglick Feb 11, 2016

Member

Use a mock instead?

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Mar 11, 2016

Unmergeable @oleg-nenashev.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Mar 11, 2016

@jglick Yes, I should spend some time and finally integrate it

…32359

Conflicts:
	src/test/java/com/cloudbees/hudson/plugins/folder/FolderTest.java
<artifactId>matrix-auth</artifactId>
<version>1.3</version>
<scope>test</scope>
<type>jar</type>

This comment has been minimized.

Copy link
@jglick

jglick Jun 8, 2016

Member

redundant

<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>matrix-auth</artifactId>
<version>1.3</version>
<scope>test</scope>

This comment has been minimized.

Copy link
@jglick

jglick Jun 8, 2016

Member

Not exactly happy to see this come back, but I guess in test scope it is OK, and you needed two distinct AbstractFolderProperty implementations I suppose?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 10, 2016

Author Member

Yes. I could create Mock ones, but a realistic case slightly improves the test coverage

* Gets an owner of the property.
* @return Owner of the property.
* It should be always non-null if the property initialized correctly.
* @since TODO

This comment has been minimized.

Copy link
@jglick

jglick Jun 8, 2016

Member

🐜 Would rather not see this be added to the API. There is no use case for it.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 10, 2016

Author Member

I'll add restricted

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jun 8, 2016

🐝 with some comments

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jun 10, 2016

re🐝

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jun 13, 2016

@oleg-nenashev oleg-nenashev changed the title [FIXED JENKINS-32359,JENKINS-32487] - Properly assign persisted list owners when loading folders from disk [FIXED JENKINS-32359] - Properly assign persisted list owners when loading folders from disk Jun 13, 2016
@oleg-nenashev oleg-nenashev merged commit bd37926 into jenkinsci:master Jun 13, 2016
1 check passed
1 check passed
Jenkins This pull request looks good
Details
xmorales added a commit to xmorales/cloudbees-folder-plugin that referenced this pull request Sep 1, 2016
…ading folders from disk (jenkinsci#32)

* [JENKINS-32359] - Direct Unit test for the issue

* [FIXED JENKINS-32359] - Assign persisted list owners when reloading the folder with properties

* [JENKINS-32487] - Expose AbstractPropertyFolder's owner via API

* [JENKINS-32487] - Test suite refactoring

* [JENKINS-32359] - Address comments from @jglick

* [JENKINS-32359] - Add test dependency on matrix-auth plugin

* [JENKINS-32359] - Restrict AbstractFolderProperty#getOwner()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.