Skip to content

fix void element is a simple element#34

Open
amairi wants to merge 1 commit into
masterfrom
fix_void_element_is_simple_element
Open

fix void element is a simple element#34
amairi wants to merge 1 commit into
masterfrom
fix_void_element_is_simple_element

Conversation

@amairi

@amairi amairi commented Oct 29, 2019

Copy link
Copy Markdown
Collaborator

example of void element :
<any:element xmlns:any="http://foo.bar.net/-/any/"/>

Co-authored-by: Brahim Amairi brahim.amairi@gmail.com
Co-authored-by: Farid Chetouani farid.chetouani@vidal.fr

example of void element :
<any:element xmlns:any="http://foo.bar.net/-/any/"/>

Co-authored-by: Brahim Amairi <brahim.amairi@gmail.com>
Co-authored-by: Farid Chetouani <farid.chetouani@vidal.fr>
@AElMehdiVidal

AElMehdiVidal commented Oct 29, 2019

Copy link
Copy Markdown
Contributor

L'exemple que vous essayez de corriger il correspond bien à un structured extension element,
l'erreur vient plutôt de l'implémentation de la spec du simple element. Il y a une issue ouverte la dessus.

Structured Extension Element: The root element of a Structured Extension element MUST have at least one attribute or child element. Ce qui est bien le cas dans votre exemple.

Simple Extension Element: A Simple Extension element MUST NOT have any attributes or child elements. The element MAY contain character data or be empty. Simple Extension elements are not Language-Sensitive.

PS : For those having hard time understanding the Relax-NG fragments used in the RFC, they can refer to the documentation of the RELAX-NG schema language.

Vu et écrit avec @amairi.1111

@AElMehdiVidal

AElMehdiVidal commented Oct 29, 2019

Copy link
Copy Markdown
Contributor

ping @fchetouani.

@s-petit s-petit left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ça me parait cool !

@jcgay

jcgay commented Oct 30, 2019

Copy link
Copy Markdown
Contributor

Merci pour le rappel de l'issue @AElMehdiVidal je ne l'avais plus du tout en tête 😇

Par contre à mon sens de ce que j'ai compris du cas de @fchetouani et @amairi il y a bien une correction à faire (avec la version pas respectueuse de la spec) parce que dans ce cas on marshall un SimpleElement et à l'unmarshall on obtient un StructuredElement ?!

@jcgay

jcgay commented Oct 30, 2019

Copy link
Copy Markdown
Contributor

J'ai pas repris le code mais si on ajoute un test d'un element sans attribut on obtient bien aussi un StructuredElement au lieu d'un SimpleElement ?

@jcgay jcgay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est cosmétique mais si vous pouvez enlever aussi le fix() dans le message de commit, c'est pas dans les habitudes du repository si vous regardez l'historique.

👍

}

@Test
public void unmarshal_single_simple_void_element() throws Exception {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On peut aussi ajouter un test d'un élément sans attribut ?


ExtensionElement result = unmarshalElement(xml);

assertThat(result).isEqualTo(ExtensionElements.simpleElement("element", "")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cela doit être un structured element, et ce qu'on marshall c'est aussi un structured (contient un attribut). Voir les autres échanges sur la PR

@AElMehdiVidal

AElMehdiVidal commented Oct 30, 2019

Copy link
Copy Markdown
Contributor

@jcgay,

Oui il y a bien une correction qui doit être faite, la question qui se pose est plutôt sur la piste à prendre :

  1. Corriger notre implémentation de simple element pour s'aligner avec la RFC ? Sans bien sûr impacter Arthur (Il peut toujours rester sur la version non corrigée)

  2. Garder l'implémentation actuelle en rajoutant ce fix : Un pas de plus down the rabbit hole 🐰

  3. Une autre solution un peu moins intéressante, c'est de rajouter un filtre au niveau de leur code, sans avoir à modifier l'API.

Sinon, pour la deuxième question, de mémoire; je dirai qu'il faut couvrir (au moins) les cas de tests énumérés ci-dessous. Parce que l'existence ou pas d'un attribut n'est pas le seul critère déterminant, ça dépend aussi de l'existence ou pas d'un child element (MUST have at least one attribute or child element).

  1. Un élément avec attribut : ça doit renvoyer un structured

  2. Un élément avec un child element sans attribut : ça doit renvoyer un structured

  3. Un élément sans attribut sans child element : ça doit renvoyer un simple element

Je ne sais pas si c'est clair 😅 ou pas !

@fchetouani

Copy link
Copy Markdown
Collaborator

Pour notre problème dans Gru, pour obtenir l'amm type d'une spécialité.
Le cas où la dé sérialisation renvoyait un élément structuré au lieu du simple élément attendu a été géré autrement.
En résumé, on a plus besoin de la pull request dans le cadre de l'itération Gru.

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.

5 participants