Skip to content

Fix Schema::Element required? and array? methods#206

Merged
cfis merged 1 commit intoxml4r:masterfrom
jxa:schema-element-fixes
Sep 25, 2023
Merged

Fix Schema::Element required? and array? methods#206
cfis merged 1 commit intoxml4r:masterfrom
jxa:schema-element-fixes

Conversation

@jxa
Copy link
Copy Markdown
Contributor

@jxa jxa commented Sep 9, 2023

In this commit the min_occurs and max_occurs getters were removed, saying they were "invalid". The additional message in HISTORY states:

Remove SchemaElement#minOccurs and SchemaElement#maxOccurs since they actually did not work (Charlie Savage)

Additional history here

I'm not aware of any more context surrounding the change, but those methods were still referenced by the Schema::Element's array? and required? methods. This PR adds tests for those two methods and adds simple ivar reader methods to replace those removed C methods.

In
xml4r@133096b#diff-ff32e4dff2433e31f170b6f43842c940bec1c3581a07a5e3bc44887ce973fcc0L67-L76
the `min_occurs` and `max_occurs` getters were removed, saying they were
"invalid". The additional message in
[HISTORY](https://github.com/xml4r/libxml-ruby/blob/master/HISTORY#L63C111-L63C111)
states:

> Remove SchemaElement#minOccurs and SchemaElement#maxOccurs since they actually did not work (Charlie Savage)

[Additional history here](https://github.com/search?q=repo%3Axml4r%2Flibxml-ruby+min_occurs&type=commits)

I'm not aware of any more context surrounding the change, but those
methods were still referenced by the Schema::Element's `array?` and
`required?` methods. This PR adds tests for those two methods and adds
simple ivar reader methods to replace those removed C methods.
@cfis
Copy link
Copy Markdown
Member

cfis commented Sep 19, 2023

Thanks for catching this and adding tests.

The xml2 documentation says that minOccurs and maxOccurs are deprecated and not used. See https://gnome.pages.gitlab.gnome.org/libxml2/devhelp/libxml2-schemasInternals.html

So either we should remove the require method or figure out if there is an API to check this (I don't see one, but maybe there is).

I don't think adding dummy ivars is the right approach since that will give back the wrong answer.

@jxa
Copy link
Copy Markdown
Contributor Author

jxa commented Sep 19, 2023

The xml2 documentation says that minOccurs and maxOccurs are deprecated and not used. See https://gnome.pages.gitlab.gnome.org/libxml2/devhelp/libxml2-schemasInternals.html

That is unfortunate. AFAIK those are the only way to know the intended cardinality of an element.

The commit that added those comments back in 2005 used to say /* Obsolete; this goes into particles */

The definition of the particle seems to be https://github.com/GNOME/libxml2/blob/d7cfe356500906fad0f3b3d186e1abbd5489a23c/xmlschemastypes.c#L355-L356 but I think that is private/internal? It's definitely not ideal to rely on a deprecated field, so maybe we can find a newer / future-proof way of obtaining the attribute values. Hopefully some of that history sparks an idea for you! C is definitely not my forte, but I can find some more time to move this forward with a bit of guidance.

I don't think adding dummy ivars is the right approach since that will give back the wrong answer.

They do give back the correct answer. The tests should demonstrate this, and I am using a monkey-patched version in one of my production applications, hoping that this PR or something similar will be merged eventually.

@cfis
Copy link
Copy Markdown
Member

cfis commented Sep 20, 2023

Yes, I saw those fields on xmlSchemaParticle but didn't see how to get access to that from an xmlSchemaElement. Thanks for the link to that commit though.

Where is @min and @max set that the Ruby code is returning?

@jxa
Copy link
Copy Markdown
Contributor Author

jxa commented Sep 20, 2023

Great question, thanks for the prompt!

ruby_xml_schema_type.c is using a reference to a Particle to set those ivars on Element. So it seems my first reading of the xmlSchemaParticle was incorrect. That is a public interface, and indeed it is what libxml-ruby is using to construct the Element in ruby-land.

@jxa
Copy link
Copy Markdown
Contributor Author

jxa commented Sep 25, 2023

As for the one failing test on windows-latest, do you think it might be a flaky test? I don't see how it would be related to my changes. Is is possible to rerun that particular check? I don't see a way but maybe I just don't have the proper permissions.

@cfis
Copy link
Copy Markdown
Member

cfis commented Sep 25, 2023

Yes, passed the second time.

@cfis cfis merged commit c7b46b0 into xml4r:master Sep 25, 2023
@cfis
Copy link
Copy Markdown
Member

cfis commented Sep 25, 2023

Thanks for fixing this.

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.

2 participants