Skip to content

Conversation

@tooomm
Copy link
Contributor

@tooomm tooomm commented Oct 12, 2020

fixes #65

Travis CI will now automatically check and report if tokens.xml is valid against Cockatrice DB schema v4, and challange-tokens.xml against Cockatrice DB schema v3.

On every change made to the repo or any PR created this will trigger and has results shortly after.

Checks are super fast, but it looks like we are not perfectly valid currently?

@tooomm
Copy link
Contributor Author

tooomm commented Oct 12, 2020

From the Travis logs:

$ xmllint --schema cards-v3.xsd --noout challenge_tokens.xml

challenge_tokens.xml:104: element card: Schemas validity error : Element 'card': Character content other than whitespace is not allowed because the content type is 'element-only'.

challenge_tokens.xml fails to validate

The command "xmllint --schema cards-v3.xsd --noout challenge_tokens.xml" exited with 3.
$ xmllint --schema cards-v4.xsd --noout tokens.xml

tokens.xml:7: element cockatrice_carddatabase: Schemas validity error : Element 'cockatrice_carddatabase', attribute '{https://www.w3.org/2001/XMLSchema-instance}noNamespaceSchemaLocation': The attribute '{https://www.w3.org/2001/XMLSchema-instance}noNamespaceSchemaLocation' is not allowed.

tokens.xml:1361: element card: Schemas validity error : Element 'card': Character content other than whitespace is not allowed because the content type is 'element-only'.

tokens.xml:5167: element prop: Schemas validity error : Element 'prop': This element is not expected. Expected is one of ( token, tablerow, cipt, upsidedown ).

tokens.xml:5602: element reverse-related: Schemas validity error : Element 'reverse-related': This element is not expected. Expected is one of ( tablerow, cipt, upsidedown ).

tokens.xml:7499: element set: Schemas validity error : Element 'set', attribute 'pickURL': The attribute 'pickURL' is not allowed.

tokens.xml:8291: element reverse-related: Schemas validity error : Element 'reverse-related': This element is not expected. Expected is one of ( tablerow, cipt, upsidedown ).

tokens.xml fails to validate

The command "xmllint --schema cards-v4.xsd --noout tokens.xml" exited with 3.

@tooomm tooomm mentioned this pull request Oct 12, 2020
@tooomm
Copy link
Contributor Author

tooomm commented Oct 12, 2020

Tried to fix the obvious errors already in #94.
Also revealed some sneaky typos... I like this automated check already. 😄👍

Please have a look at the spotted validation errors in this PR's and apply any further changes to #94 @Psithief!

Edit: more fixes in #96

@Psithief
Copy link
Contributor

As per Cockatrice/Cockatrice#2787 and Cockatrice/Cockatrice#3710

The previously pushed XSD does not enforce at least one appearance any element that was required. It's not practical to implement this without a sequence with XML1.0 for this many elements.

Unfortunately it's not perfect; the most problematic point is that some elements in our xml are not used as xml would like to:

  • multiple set-per-card definitions are not nested inside a parent element;
  • the same for card relations and ;
    This forces us into using xs:sequence instead of xs:all, that unfortunately also enforces the order of elements.

These are mostly XSD 1.0 limitations that could be lifted by using an XSD 1.1 validator, but afaik no decent XSD 1.1 validator is easily available on every os/platform.

If we're automating checks, we really should try to revisit using an XML 1.1 XSD so we don't get complaints about a perfectly valid XML file.

@Psithief
Copy link
Contributor

Checks are super fast, but it looks like we are not perfectly valid currently?

No, if it runs in Cockatrice, then the file is perfectly valid. The XSD is incomplete and incorrect documentation that defines a subset of the XML schema that will work with Cockatrice.

If we merge this, the checks are just going to be ignored because they can't be trusted. Once that happens, you will find it difficult to get that trust back if a more accurate tool is implemented later.

@tooomm
Copy link
Contributor Author

tooomm commented Oct 18, 2020

Sure, that makes sense. As long as the checks are not helpful we should not merge this.

If we're automating checks, we really should try to revisit using an XML 1.1 XSD so we don't get complaints about a perfectly valid XML file.

So the XSD at Cockatrice needs another update?

@tooomm tooomm changed the title Enable Travis CI to validate our XML files against Cockatrice XSD on every change [DO NOT MERGE] Enable Travis CI to validate our XML files against Cockatrice XSD on every change Oct 18, 2020
@tooomm tooomm changed the title [DO NOT MERGE] Enable Travis CI to validate our XML files against Cockatrice XSD on every change [DO NOT MERGE] Use GitHub Actions to validate our XML files against Cockatrice XSD on every change Mar 3, 2021
@tooomm
Copy link
Contributor Author

tooomm commented Mar 3, 2021

I revisited this PR to update it in order to use GitHub Actions over Travis to stay in line with all CI stuff in the Cockatrice organisation. It integrates better with the rest of the GitHub page and don't take the users away when reviewing PR's. It's all right in the Actions tab in every PR and at the top of the repo.


Outdated text

See #93 (comment)

Had another look. While the error message by xmllint in CI looks like this:

tokens.xml:7: element cockatrice_carddatabase: Schemas validity error : Element 'cockatrice_carddatabase', attribute '{https://www.w3.org/2001/XMLSchema-instance}noNamespaceSchemaLocation': The attribute '{https://www.w3.org/2001/XMLSchema-instance}noNamespaceSchemaLocation' is not allowed.

Using e.g. https://www.freeformatter.com/xml-validator-xsd.html, the error message is a bit more helpful:

Cvc-complex-type.3.2.2: Attribute 'xsi:noNamespaceSchemaLocation' Is Not Allowed To Appear In Element 'cockatrice_carddatabase'., Line '6', Column '218'.

In the XSD, only the version attribute is defined in the cockatrice_carddatabase element.
Do we need the xsi:noNamespaceSchemaLocation attribute in the token.xml file? It got added in this commit.
If yes, the XSD at Cockatrice should probably get updated to define it?



The XSD is incomplete and incorrect documentation that defines a subset of the XML schema that will work with Cockatrice.

Somebody with better XML/XSD knowledge needs to help here. :)


This does not change Psi's point that this should not be merged until the XSD is updated and this check can be trusted. Which requires the XSD to be updated to 1.1.

These are mostly XSD 1.0 limitations that could be lifted by using an XSD 1.1 validator, but afaik no decent XSD 1.1 validator is easily available on every os/platform.

There is xmlschema, a Python library, with XSD 1.1 support!

@tooomm tooomm marked this pull request as draft July 3, 2021 17:21
@tooomm
Copy link
Contributor Author

tooomm commented Jul 11, 2021

Note to myself

Problem matcher works nicely and annotates + adds link to the Action overview:
problem matcher

Also a great view on the changed files tab:
2

@tooomm
Copy link
Contributor Author

tooomm commented Jul 11, 2021

The last mystical

tokens.xml:7: element cockatrice_carddatabase: Schemas validity error : Element 'cockatrice_carddatabase', attribute '{https://www.w3.org/2001/XMLSchema-instance}noNamespaceSchemaLocation': The attribute '{https://www.w3.org/2001/XMLSchema-instance}noNamespaceSchemaLocation' is not allowed.

error was due to xmlns:xsi not liking https for some reason. c73fcaf (#93)

@tooomm
Copy link
Contributor Author

tooomm commented Jun 2, 2022

Example of the xmllint run introduced here being useful:
#134 introduced an issue, which got spotted 4 month later solely by investigating an broken token reported at Cockatrice.
Fixed in #150.

The CI run would have reported the issue in the introducing PR right away before even merging: picUrl instead picURL

Error: tokens.xml:2104: element set: Schemas validity error : Element 'set', attribute 'picUrl': The attribute 'picUrl' is not allowed.
Error: tokens.xml:2692: element set: Schemas validity error : Element 'set', attribute 'picUrl': The attribute 'picUrl' is not allowed.
Error: tokens.xml:3681: element set: Schemas validity error : Element 'set', attribute 'picUrl': The attribute 'picUrl' is not allowed.
tokens.xml fails to validate

Note:
In the example above, the challenge file is not checked, probably because the other step failed already. fail-fast: false?

@tooomm tooomm changed the title [DO NOT MERGE] Use GitHub Actions to validate our XML files against Cockatrice XSD on every change Validate token XML files against Cockatrice's XSD schema Jun 21, 2022
@tooomm tooomm changed the title Validate token XML files against Cockatrice's XSD schema CI: Validate token XML files against Cockatrice's XSD schema Jun 21, 2022
@SlightlyCircuitous
Copy link
Contributor

SlightlyCircuitous commented Mar 27, 2023

The XSD is incomplete and incorrect documentation that defines a subset of the XML schema that will work with Cockatrice.
Somebody with better XML/XSD knowledge needs to help here. :)

My understanding is that our cards.xsd files care about the order of elements in an entry, whereas Cockatrice doesn't really care as long as they are present. For example, #96 rearranged two lines to make the validator happy, but Cockatrice can display all that information correctly either way. Based on Psithief's explanation, this seems to be necessary in our case because of the constraints of XSD 1.0. I agree that if this validator flagged element order issues frequently, it would get annoying and soon be ignored since the file is valid in Cockatrice.

However, this seems like it would be a pretty rare false positive nowadays as we basically always put elements in the same order. cards.xml is generated automatically (right?), my updates to tokens.xml are generated near enough to automatically (and it seems like other people copy and paste the blank template if they do it manually so order is still preserved), and the challenge tokens will probably never be updated again (besides the proposition to swap them to v4, which was also done automatically).

It sounds like it would definitely be advantageous to switch to XSD 1.1 at some point, but, from what I have read in this PR, it doesn't seem like not being able to make that switch should block automatic XML validation. Of course, that is just based on this element ordering issue. Maybe there are other potentially annoying false positives that I am not aware of.

@ZeldaZach ZeldaZach closed this Nov 1, 2023
@tooomm tooomm deleted the tooomm-test branch April 1, 2025 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI: Check xml file formatting and validate against xsd

4 participants