[IMP] load_data: allow to modify xml file before loading

[hbrunn]

since we started automatically generating the analysis for openupgrade, it bothers me that we have to do changes to xml files in a copy of the generated file.

So here I propose to allow users of openupgradelib to change the file while loading, this way there's no need to change stuff in the file at all, and if the changes are crafted sufficiently defensively, they should survive any subsequent changes of the noupdate_changes.xml file.

With this, ie in the base module instead of copying the changes file, we write

openupgrade.load_data(
    env, "base", "19.0.1.3/noupdate_changes.xml",
    xml_transformation="""
    <xpath expr="//record[@id='user_admin']" position="replace" />
    <xpath expr="//record[@id='user_root']" position="replace" />
    """
)

This also allows us to load some noupdate changes conditionally, as in the case I'm working on currently: Update a product's UOM if it's not used in invoices already, don't otherwise (that's not what I ended up doing, but it's a possibility anyways)

[legalsylvain]

Hi.

Thanks a lot for the work. I'm in favor of such move.
I think it will make easier the the maintenance work of openupgraders.

I have an idea to maybe "improve" your proposal. maybe it's not a good one, let me know !

Your proposal
an xml content is written in a python file : As a result, we can not easely check if xml syntax is correct, we can not apply pre-commit xml check, reviewer can not have syntaxic collorations, maybe pep8 line88 constraint will be sometime anoying, etc.

My proposal
openupgradelib changes pseucode / pseudocomment :

    def load_data(env_or_cr, module_name, filename, idref=None, mode="init"):
        # given a filename "BASE_FILENAME.xml"
        # check if a file named "BASE_FILENAME.patches.xml" exists
        # if yes, load it and apply transformations to BASE_FILENAME.xml
        # Then, classic load_data algorithm.

exemple of noupdate_changes.patches.xml in your exemple:

    <?xml version="1.0" encoding="UTF-8"?>
    <transformations>
         <!-- comment 1: why i want to disable this section -->
         <xpath expr="//record[@id='user_admin']" position="replace" />
         <!-- comment 2: ... -->
         <xpath expr="//record[@id='user_root']" position="replace" />
    </transformations>

Note :
we can make "implicit loading" = if xxx.patches.xml exists, load it OR we can make explicit loading :

def load_data(env_or_cr, module_name, filename, idref=None, mode="init"; transformation_filename=None):

[hbrunn]

thanks @legalsylvain, see your point. I'll prefer passing xml_transformation_filename that's treated the same way as filename, any objections?

[legalsylvain]

thanks @legalsylvain, see your point. I'll prefer passing xml_transformation_filename that's treated the same way as filename, any objections?

I think that the option you want is in the "spirit" of OpenUpgrade. I remember a few years ago, a discussion about 2 options :

• if a noupdate_changes.xml exists, load it automatically
• versus : call explicitely "load_data" function

-> and the result was "better to be explicit". so in the same way, better to add a xml_transformation_filename parameter.

A single question : you talk about conditional loading in the first comment. If it's an interesting feature, maybe it's better to implement a xml_transformation_filenames=[] parameter that accepts one or many files ?
something that allows some call like that :

xml_transformation_filenames=["noupdate_changes.generic_patches.xml", "noupdate_changes.product_uom_patch.xml"]

[hbrunn]

for that use case I'll rather have this support a file-like object, that the user fills however they see fit. but let's not overthink this, just a filename will be fine for now

[legalsylvain]

@OCA/openupgrade-maintainers : any extra PoV about that topic ?
thanks !

[StefanRijnhart]

@legalsylvain I'd be happy with the xml_transformation_filename option.

[hbrunn]

thanks all, I've updated the code accordingly and the PR using this is indeed nicer now.

Wrong XML there then also is detected in checks as expected:

check xml................................................................Failed
- hook id: check-xml
- exit code: 1

openupgrade_scripts/scripts/hr_expense/19.0.2.1/noupdate_changes-transformation.xml: Failed to xml parse (openupgrade_scripts/scripts/hr_expense/19.0.2.1/noupdate_changes-transformation.xml:14:2: mismatched tag)

[legalsylvain]

Hi @hbrunn. I don't understand why xml-check fail with the xml you propose.
https://github.com/OCA/OpenUpgrade/pull/5675/changes#diff-8a7dc48ec4613364962aab780e7de83b80af4460df7c7e2fa589b3541268bf9aR1-R14

it is valid ? Or did i missed something ?

[hbrunn]

I deliberately added a mistake, which I didn't commit of course

[legalsylvain]

I deliberately added a mistake, which I didn't commit of course

sorry, I misread your comment.

all is OK, then ! thanks !