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

XML Prolog is omitted in saved file #1163

Open
danyill opened this issue Feb 7, 2023 · 10 comments
Open

XML Prolog is omitted in saved file #1163

danyill opened this issue Feb 7, 2023 · 10 comments
Labels
Kind: Bug - Core Priority: Important Tackle eventually Status: Pair needed Developers need business expert

Comments

@danyill
Copy link
Collaborator

danyill commented Feb 7, 2023

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 danyill added the Kind: Bug Something isn't working label Feb 7, 2023
@danyill
Copy link
Collaborator Author

danyill commented Feb 7, 2023

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
Copy link
Collaborator Author

danyill commented Feb 7, 2023

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.

actions.push({
new: {
parent: this.doc!.querySelector(':root')!,
element: ied,
},
});
this.dispatchEvent(
newActionEvent({
title: get('editing.import', { name: ied.getAttribute('name')! }),
actions,
})
);

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.

@Sander3003
Copy link
Member

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

@danyill
Copy link
Collaborator Author

danyill commented Feb 8, 2023

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
Copy link
Collaborator Author

danyill commented Feb 8, 2023

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):

open-scd/src/Editing.ts

Lines 427 to 435 in e22e42d

const newDoc = document.implementation.createDocument(
this.doc.lookupNamespaceURI(''),
this.doc.documentElement.tagName,
this.doc.doctype
);
// change the document object reference to enable reactive updates
newDoc.documentElement.replaceWith(this.doc.documentElement);
this.doc = newDoc;

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

@ca-d
Copy link
Contributor

ca-d commented Feb 14, 2023

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!

danyill added a commit to danyill/open-scd that referenced this issue Feb 14, 2023
danyill added a commit to danyill/open-scd that referenced this issue Feb 14, 2023
danyill added a commit that referenced this issue Feb 27, 2023
Ensure XML declaration is present on save, part of #1163

---------

Co-authored-by: cad <christian.dinkel@omicronenergy.com>
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

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 github-actions bot added the stale label Sep 6, 2023
@danyill
Copy link
Collaborator Author

danyill commented Sep 6, 2023

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.

@github-actions github-actions bot removed the stale label Sep 7, 2023
@michelguerin michelguerin added Kind: Bug - Core and removed Kind: Bug Something isn't working labels Nov 8, 2024
@michelguerin michelguerin added the Status: Pair needed Developers need business expert label Nov 8, 2024
@michelguerin
Copy link
Contributor

@danyill is this issue still relevant ?

@danyill
Copy link
Collaborator Author

danyill commented Nov 9, 2024

@danyill is this issue still relevant ?

Well.... maybe. I guess it's still relevant as the Save plugin works around the issue that the Action API throws away the prolog which could cause issues with any other tool which saves or modifies XML files.

Once the Action API is gone then the issue will be resolved as I don't believe it occurs with the newer APIs.

So we could close it because we have an upgrade path that will resolve it naturally or we could leave it open until that is done (my preference).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Kind: Bug - Core Priority: Important Tackle eventually Status: Pair needed Developers need business expert
Projects
Status: Todo
Development

No branches or pull requests

7 participants