open-scd icon indicating copy to clipboard operation
open-scd copied to clipboard

XML Prolog is omitted in saved file

Open danyill opened this issue 2 years ago • 8 comments

Describe the bug

When importing an ICD and an IID file into a new Edition 2 project and then saving, the XML prolog is omitted (this caused the GE Enervista software to crash). We think it is also non-compliant with Part 6, Ed 2.1, section 8.4.

To Reproduce

Steps to reproduce the behavior:

  1. Go to New Project
  2. Click on Edition 2
  3. On the menu go to Import IEDs
  4. Import both IEDs in the attached zip file
  5. Save the project
  6. Open in an XML editor
  7. Observe there is no XML prolog

B30 (1).zip

Expected behavior

The XML prolog is always included:

image

Also

This doesn't seem to be related to the input files but rather the way we instantiate a "New Project".

The code looks like it supports the XML prolog, but perhaps it is not sufficient to generate it from a string?

export function newEmptySCD(id, versionId) {
  const {version, revision, release} = supportedAttributes[versionId];
  const markup = `<?xml version="1.0" encoding="UTF-8"?>
    <SCL xmlns="http://www.iec.ch/61850/2003/SCL" ${version ? `version="${version}"` : ""} ${revision ? `revision="${revision}"` : ""} ${release ? `release="${release}"` : ""}>
      <Header id="${id}"/>
    </SCL>`;
  return new DOMParser().parseFromString(markup, "application/xml");
}

In the Communication Export plugin I did:

    const sclNamespace = "http://www.iec.ch/61850/2003/SCL";
    const sclDoc = document.implementation.createDocument(sclNamespace, "SCL", null);
    const pi = sclDoc.createProcessingInstruction("xml", 'version="1.0" encoding="UTF-8"');
    sclDoc.insertBefore(pi, sclDoc.firstChild);

Which seems to generate the correct prolog.

danyill avatar Feb 07 '23 17:02 danyill

I think this is because the XML prolog is not XML, strictly speaking. It has to be added in a different way, this seemed helpful to me:

https://stackoverflow.com/questions/50109538/altering-an-existing-xml-prolog-with-javascript

danyill avatar Feb 07 '23 17:02 danyill

It is quite obscure but it is not the way the prolog is formed:

  • Creating a new (empty) Edition 2 project and saving it, it contains the prolog
  • When creating a new project and combining both on my Windows machine (using Chrome) it does not contain the prolog.
  • When creating a new project and combining both on my Linux machine (using Firefox or Chrome) it does not contain the prolog.
  • When creating a new project and only using the 'NR.icd' in "Import IEDs" it does not contain the prolog (Firefox)
  • When creating a new project and only using the 'B30.iid' in "Import IEDs" it does not contain the prolog (Firefox)
  • It appears that something about the combined file after "Import IEDs" is used is problematic.
  • If I open either 'NR.icd' or 'B30.iid' and then save it (without using New Project) the prolog is present.

So it seems to be narrowing it down to something related to what happens in "Import IEDs" I thought it might be the difference between importNode / cloneNode but it doesn't appear to be (although it could be in the editing/action API).

I put a breakpoint on L440 before the action to import the IED occurred, and imported both IEDs.

https://github.com/openscd/open-scd/blob/e22e42db4d89ea0161b2ea3db3b7ea5f38761423/src/menu/ImportIEDs.ts#L433-L445

Before I get:

new XMLSerializer().serializeToString(this.doc).slice(0,100)
'<?xml version="1.0" encoding="UTF-8"?><SCL xmlns="http://www.iec.ch/61850/2003/SCL" version="2007" r'

after I get:

new XMLSerializer().serializeToString(this.doc).slice(0,100)
'<SCL xmlns="http://www.iec.ch/61850/2003/SCL" version="2007" revision="B" release="4" xmlns:xsi="htt'

I wonder if the new editing API has a similar problem, I'll look to see exactly where the breakage occurs in some time.

danyill avatar Feb 07 '23 18:02 danyill

Suggestion: maybe we can add an automated test to this in order to see if the behavior is correct.

Sander3003 avatar Feb 08 '23 07:02 Sander3003

I think the core problem resides in the action API.

I'm not sure how best to diagnose it. A workaround which can readily be applied might be to do the following in the "Save Project" which is effectively hunt for the relevant prolog-related item if it's not there, add the prolog.

This would prevent further work on the old API:

export default class SaveProjectPlugin extends LitElement {
  async run() {
    if (this.doc) {
      // at prolog if it's been stripped
      if (this.doc.xmlEncoding === null) {
        const pi = this.doc.createProcessingInstruction(
            'xml',
            'version="1.0" encoding="UTF-8"'
        );
        this.doc.insertBefore(pi, this.doc.firstChild);
      }

If this is acceptable I can do a PR.

danyill avatar Feb 08 '23 08:02 danyill

We decided to see if progress in using the new edit action API will resolve this in a short time (next couple of weeks).

The root cause is that at the moment in the action API we instantiate a new document constantly without adding the prolog (thanks @ca-d):

https://github.com/openscd/open-scd/blob/e22e42db4d89ea0161b2ea3db3b7ea5f38761423/src/Editing.ts#L427-L435

This was broken about 4-6 months ago as part of improving the update cycle for plugins.

danyill avatar Feb 08 '23 09:02 danyill

export default class SaveProjectPlugin extends LitElement {
  async run() {
    if (this.doc) {
      // at prolog if it's been stripped
      if (this.doc.xmlEncoding === null) {
        const pi = this.doc.createProcessingInstruction(
            'xml',
            'version="1.0" encoding="UTF-8"'
        );
        this.doc.insertBefore(pi, this.doc.firstChild);
      }

If this is acceptable I can do a PR.

I'm currently in favor of this solution as a quick fix, since technical questions have come up for tomorrow's refinement which bear on our edit action migration timeline. If you could find the time to submit it after all, I'd try to review it as quickly as possible. Thank you!

ca-d avatar Feb 14 '23 12:02 ca-d

Hello there,

Thank you for opening this issue! We appreciate your interest in our project. However, it seems that this issue hasn't had any activity for a while. To ensure that our issue tracker remains organized and efficient, we occasionally review and address stale issues.

If you believe this issue is still relevant and requires attention, please provide any additional context, updates, or details that might help us understand the problem better. Feel free to continue the conversation here.

If the issue is no longer relevant, you can simply close it. If you're uncertain, you can always reopen it later.

Remember, our project thrives on community contributions, and your input matters. We're here to collaborate and improve. Thank you for being part of this journey!

github-actions[bot] avatar Sep 06 '23 19:09 github-actions[bot]

We contemplate the benefits of having bots make people work.

This issue is still here even though the underlying issue is now resolved because the "workaround" code which ensures the prolog is there should be removed as it is most likely just cruft at this point. I think this means #1173 could be reverted and a few tests would be required to see this issue was still resolved.

danyill avatar Sep 06 '23 19:09 danyill