Apply profile dependencies only once.

[mauritsvanrees]

Example scenario:

1. Create a Plone 5 beta site. This applies a profile from plone.app.contenttypes.
2. Install add-on Mosaic. This lists plone.app.contenttypes in its metadata.xml, so the profile gets applied a second time.

Problems with this scenario:

1. This profile needlessly gets applied twice. Takes extra time. Oh well.
2. If before installing Mosaic you made some manual changes to the plone.app.contenttypes settings, or installed another add-on that did this, then those changes will be undone. Well, this depends on the exact changes, but I think you can imagine.
3. Theoretically, plone.app.contenttypes might do something stupid in an import step that gives an exception when run a second time, or that now creates twice as many sample content or whatever.

I would like to prevent this. So this is the change:

Dependency profiles from metadata.xml that are already applied, are not applied again. Instead, its upgrade steps, if any, are applied. In code you can choose the old behavior of always applying the dependencies, by calling runAllImportStepsFromProfile with always_apply_profiles=True. Or you can choose to be happy with any applied version and ignore any upgrade steps, by using upgrade_dependencies=False. Note that these arguments cannot both be True.

Note: I could theoretically enhance metadata.xml so you can tweak the behavior:

<metadata>
  <version>1.0</version>
  <dependencies upgrade_dependencies="False">
    <dependency>profile-other:foo</dependency>
    <dependency always_apply_profiles="True">profile-other:bar</dependency>
    <dependency upgrade_dependencies="True">profile-other:quux</dependency>
  </dependencies>
</metadata>

But this would require changing the output of getDependenciesForProfile and getProfileInfo, so probably best not to do that.

While doing this, I got confused about which GenericSetup method needs a profile id with profile- at the start, and which needs a profile id without it. So I added a second change:

Be more forgiving when dealing with profile ids with or without profile- at the start. All functions that accept a profile id argument and only work when the id does not have this string at the start, will now strip it off if it is there. For example, getLastVersionForProfile will give the same answer whether you ask it for the version of profile id foo or profile-foo.

These two changes of this pull request are in two commits, with minimal overlap, so if needed it should be fairly easy to cherry-pick only one.

[Themanwithoutaplan]

Can we test this without having to worry about Plone?

[Themanwithoutaplan]

Minor quibble but what's with len('profile-')? profile_id[8:] would make more sense to me as this is not dynamic.

[mauritsvanrees]

For me the len('profile-') makes the intent more explicit. When checking whether the code is correct, you immediately see that this does the correct thing. With len(8) I would start counting the number of characters in profile- to see if those are really 8, and then do a recount to double check it.

At least, that is my reasoning. I don't mind changing it if wanted.

[Themanwithoutaplan]

You have the same code repeatedly, which smells of possible typos. Tests will determine whether it's working correctly and is likely to flag up errors precisely because of the fragility.

The alternative would be to use a regex for 'profile-|snapshot-'

[mauritsvanrees]

I don't see len(8) being less fragile than `len('profile-').

Code like this has been here for years, see for example:
https://github.com/zopefoundation/Products.GenericSetup/blame/1.7.7/Products/GenericSetup/tool.py#L839
https://github.com/zopefoundation/Products.GenericSetup/blame/1.7.7/Products/GenericSetup/tool.py#L998

[mauritsvanrees]

Well, looking more closely, your comment was about two lines that both had profile-, but after those lines are two similar lines with snapshot-. I guess that was your point, but it got lost because I did not see those last two lines on github when reading your comment.
I'll make the code a bit clearer. :-)

[mauritsvanrees]

Done in commit b9fe932.

[Themanwithoutaplan]

The point was that x[8:] is more fragile than x[len(somtheing):] So snapshot- and profile- don't get muddled by accident. I've not reviewed GenericSetup before so I'm bound to pick up on stuff.

[mauritsvanrees]

@Themanwithoutaplan The example I give is for some Plone packages, but the same is true outside of Plone.

[Themanwithoutaplan]

Surely always_apply_profiles should be outside the loop?

If chain has a length then for profile_id in chain[:-1]: should work too, shouldn't it?

[mauritsvanrees]

No, always_apply_profiles needs to be inside the loop. If it is False, then we do not need the check in the following lines, but can simple execute the original lines at the end of the loop (context =...; self.applyContext(context)).

I need the enumerate because I need the index. Note that the last profile in the chain is the profile that you originally wanted to install (by selecting it in portal_setup / Import tab) and the earlier ones in the chain are its dependencies. For those dependencies we need the always_apply_profiles check, for the original one not, because the user has explicitly requested to apply this profile.

[do3cc]

@Themanwithoutaplan Can you elaborate about why you think Plone is something that does need to be tought of then working on Genericsetup?

[Themanwithoutaplan]

@do3cc I just want to be able to check this without reference to Plone.

[do3cc]

@Themanwithoutaplan Oh, got you now :-)

[do3cc]

@mauritsvanrees You obviously have spent quite some time thinking about the implications of the changes, which I very much like, btw.
I try to understand the implications of your changes.
Is this correct?
After you change, best practice would be:
After release, the only file of a profile that should get updated is the metadata.xml file, with versions.
Changes should be made in upgrade steps.
Upgrade steps should NOT load profile steps.

[mauritsvanrees]

@Themanwithoutaplan Regarding the last comment, I think you are just missing that these two lines are still inside the loop. The should make the logic clearer.

[Themanwithoutaplan]

Thanks, the logic is still a bit weird but makes more sense now. This is the sort of method where I'd normally spend time working out how to reduce the number of branches in it but sometimes life is too short.

[do3cc]

That index != last_index means, not dependent profile is not easily visible from these lines of code

[mauritsvanrees]

Fair enough. I have added an explanation in comments in 3f93d07.

[mauritsvanrees]

@do3cc It is fine if an upgrade step has code to apply a specific import step or a complete profile, either its own profile or a third party profile. But the idea is: if you apply a complete profile (in code or ZMI or a control panel) and this profile has dependency profiles that are already applied, then with this pull request these dependency profiles are no longer reapplied. In case those dependency profiles have any upgrade steps that need to be applied, then those upgrade steps are applied.

There is no need for changing best practices.

[do3cc]

@mauritsvanrees Currently, my upgrade steps often just load a step from my default profile, because profiles get f uped all the times anyway...

[do3cc]

I'd be willing to press the merge button, but wait for @Themanwithoutaplan responses

[mauritsvanrees]

@do3cc Applying import steps within an upgrade step is fine in itself. I do it too. This pull request changes nothing about that. You just always need to watch out that this does not override manual changes or changes made by other packages, like inadvertently resetting properties to their defaults. If that is a danger in the package you are working on, you can always write python code in the upgrade step. The best practice will depend on your use case, and of course you have less worry if this is only an internal package for one site of one client.

[Themanwithoutaplan]

The number of codepaths in _runImportStepsFromContext() makes me dizzy as does the apparent dependence upon a mutable keyword arg (seen), which otherwise seems to have no function in the method. The proposed changes add to the complexity. The changes of themselves are minor though using enumerate(seq, 1) would remove the need for the clumsy idx - 1 but I'd be much happier with a refactoring of the major loops into distinctly testable methods or functions. Lipstick on a pig, I know but this is going to be hell if it ever needs debugging.

Take the opportunity to look a bit more around the code. The parser is still using SAX. :-(

[do3cc]

@mauritsvanrees This changes my best practice because before it simply was not possible to have good practices :-)

[do3cc]

@Themanwithoutaplan I am all in favour of declaring the currently existing complexity a bug. But I fear nobody is going to work on it and it would be unfair to block the PR because of it. What is your suggestion?

[Themanwithoutaplan]

@do3cc I think that at least the new codepaths should be in separate methods but I'd be tempted to go further. The possible dependency on the side-effects of a mutable keyword arg (as opposed to an instance variable) makes me physically unwell.

[do3cc]

@Themanwithoutaplan Ah, you didn't see the places where attribute getter trigger changes then :-)

[Themanwithoutaplan]

No, I spent 10 minutes trying to grok the original method and then the changes to it. We've got attribute magic as well to contend with? Oh joy!

I suspect much of the code could probably be simplified but I also suspect, like you, that no one can be bothered.

[do3cc]

@Themanwithoutaplan If @mauritsvanrees Won't say anything, I'll clean up the method during the week

[mauritsvanrees]

@Themanwithoutaplan I did not know about enumerate(iterable, 1) to let it start with one instead of zero. Thanks for the tip. Done.

I noticed that the seen keyword argument in _runImportStepsFromContext was not actually used. It is only used in getProfileDependencyChain because that method recursively builds up this list of seen profiles. I have removed it from _runImportStepsFromContext.

Putting my new code path in a separate method is not going to work, I think. In _runImportStepsFromContext we do this:

1 Gather a chain of profiles.
2. For each profile, depending on the keyword arguments, either do nothing or apply its upgrade steps or apply the profile.
3. Gather data for returning.

Actually, seems good to add this in comments in that method. Certainly clarification is good. Done.

The second item is the core one and I think it is as complex as it needs to be. We could split the entire for loop out into another method, but not parts of it.

With an extra method it becomes more complex due to all the methods that are called. Especially I do not like that we constantly have to juggle keyword arguments around. You call runAllImportStepsFromProfile with a blacklisted_steps keyword argument. This does nothing with it except pass it to _runImportStepsFromContext, which would again do nothing with it except pass it to a new method.

Well, we could split out the original part of the for loop, so starting from this line:

context = self._getImportContext(profile_id, purge_old, archive)

But then you have three methods that all need to return some messages for its caller to handle, which also feels awkward.

[mauritsvanrees]

After a small discussion on the cmf mailing list, I have updated the pull request. See last commit a8ec44f. The result is best shown with a screen shot of the Import tab. It makes the strategies available in the ZMI. And it makes it obvious which option influences which button, because the original form was unclear.

[pbauer]

👍 this is a huge improvement!

[tseaver]

The new "strategy" choices look reasonable to me. I've lost the thread about backward-compatibility, though: how will this change affect existing code which calls runAllImportStepsFromProfile()?

[mauritsvanrees]

runAllImportStepsFromProfile:

• old situation: all dependency profiles are applied
• new situation: new dependency profiles are applied, and for old (already applied) dependency profiles we run the upgrade steps

[jensens]

I really like the new flexible variant. I personally run often in problems, because the depended profiles were applied. We worked around with custom steps applying dependencies using a blacklist. This always felt wrong. Only running upgrade steps feels much cleaner and really solve the problem we worked around.

[mauritsvanrees]

No objections anymore on the CMF mailing list.
For the record: thread is here: http://comments.gmane.org/gmane.comp.web.zope.cmf/19462

[tseaver]

@mauritsvanrees Thanks for your work on this one!