Proper normalize attribute value normalization

[dralley]

closes #371

[dralley]

Suggestions :

• Move this functionality directly to Attribute
• Provide a fast path to get the raw value

TODO:

• Character reference & entity reference substitution with associated error handling
  • Figure out what the API needs to look like

[codecov-commenter]

Codecov Report

Merging #379 (538e5cd) into master (e701c4d) will increase coverage by 0.10%.
The diff coverage is 86.95%.

❗ Current head 538e5cd differs from pull request most recent head ac7b67b. Consider uploading reports for the commit ac7b67b to get more accurate results

@@            Coverage Diff             @@
##           master     #379      +/-   ##
==========================================
+ Coverage   61.37%   61.48%   +0.10%     
==========================================
  Files          20       20              
  Lines       10157    10229      +72     
==========================================
+ Hits         6234     6289      +55     
- Misses       3923     3940      +17     

Flag	Coverage Δ
unittests	61.48% <86.95%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/errors.rs	9.52% <ø> (-2.85%)	⬇️
src/escapei.rs	13.90% <0.00%> (ø)
src/reader.rs	88.36% <ø> (+0.94%)	⬆️
src/events/attributes.rs	94.12% <88.88%> (+3.45%)	⬆️
src/lib.rs	21.09% <0.00%> (-4.92%)	⬇️
src/de/escape.rs	65.15% <0.00%> (-1.28%)	⬇️
src/de/seq.rs	91.83% <0.00%> (-0.76%)	⬇️
src/se/mod.rs	93.81% <0.00%> (-0.01%)	⬇️
src/writer.rs	90.36% <0.00%> (+0.02%)	⬆️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e701c4d...ac7b67b. Read the comment docs.

[dralley]

I plan to continue working on this over the next week or two

[dralley]

@Mingun Questions:

Currently we have the functions unescaped_value, unescaped_value_with_custom_entities and their decode equivalents, that do the escaping part but don't implement the rest of the XML attribute-value-normalization spec. I'm not sure I see any reason for those to continue to exist as far as XML is concerned, but for HTML it makes some sense, as HTML only seems to do unescaping without any other normalization of the value.

1. Does that sound accurate / align with your knowledge
2. Do you think it would make more sense to stick with functional names, like normalized_value / unescaped_value and rely on the documentation to tell users what they ought to be using, or switch more descriptive names such as html_value / xml_value
3. The behavior of Attributes depends on whether or not .html is set, should we look at doing something similar here, or would that not be worth the additional complication

[Mingun]

1. I haven't studied a situation about HTML attributes, therefore, I rely on your understanding of the situation. Then, if you include references to relevant resources in the documentation, I'll be able to learn something about that
2. I think that functional names are better
3. Probably we should inverse things and introduce different types for XML / HTML attributes, then implement only relevant methods on each. This will also solve some unpleasant things in the current API -- we can change htmlity of the attributes in the middle of iteration that probably could to lead to sophisticated bugs. I would like to avoid such dangerous usage.

[dralley]

I'm basically going off of the lack of any kind of discussion of attribute value normalization in the HTML living spec, and this discussion on stackoverflow

https://html.spec.whatwg.org/multipage/dom.html#attributes
https://html.spec.whatwg.org/multipage/syntax.html#attributes-2
https://stackoverflow.com/questions/63906320/html5-attribute-value-normalization

I think that functional names are better

I agree

Probably we should inverse things and introduce different types for XML / HTML attributes, then implement only relevant methods on each. This will also solve some unpleasant things in the current API -- we can change htmlity of the attributes in the middle of iteration that probably could to lead to sophisticated bugs. I would like to avoid such dangerous usage.

Can they? It looks like all these fields are private. But, I feel like this is still the best option. There is already attributes() and html_attributes(), it's only a matter of changing the types.

The unfortunate thing will be, that the code between the two is almost the same, just enough so that it will be really annoying to duplicate.

[dralley]

@Mingun Does the normal unescape() function need to do this as well?

[Mingun]

Yes, I think so

[Mingun]

I think, it will be valuable to add examples here. Actually, probably you can convert your new tests to doc examples and that will be enough

[Mingun]

Actually, we should add a decoder parameter to that (and similar) functions and decode first before normalization.

Maybe also try another approach: introduce a new type of always UTF-8 encoded attributes and add a Attribute::decode(&self) -> Result<Utf8Attribute> function.

Instead of completely new type we could try to use const bool generic parameter (stable since 1.51, current MSRV is 1.41.1, from memchr)

[Mingun]

Yes, I think so

[Mingun]

Can they? It looks like all these fields are private.

No, they can't. I confused with the ability to disable with_checks in the middle of an iteration.

But, I feel like this is still the best option. There is already attributes() and html_attributes(), it's only a matter of changing the types.

Ok, then let's do that.

The unfortunate thing will be, that the code between the two is almost the same, just enough so that it will be really annoying to duplicate.

If you talk about make_normalized_value, you can convert it to a free function and use it in both API methods.

[Mingun]

I've fixed the algorithm and other noted things in my branch, but I do not yet consider it as finished.

[Mingun]

Maybe made long really long? A 1KB at least

[dralley]

I would hope that 1kb attribute values isn't a common thing. But you're right that it should be a bit longer.

[dralley]

I did a cursory review of the changes and it looks fine. Would you prefer the commits squashed or kept separate?

I'll address everything else tomorrow.

[Mingun]

I prefer to kept separate. It is somehow psychologically uncomfortable for review when one commit changes more than ~200 lines in each (or just several) files, even if half of them -- new tests. ¯_(ツ)_/¯

I think, that at least separating normalization method to it's own commit would be a good idea. This is pretty isolated thing which, however, is big enough. That commit needed to be updated:

• Add tests for non-ASCII input
• Introduce new error kind and return it when depth become 0. That means that we reach recursion limit
• (could be postponed) I would like to have limit configurable
• (could be postponed) Add a way to explicitly detect recursion (i.e. track the resolved entities and report which entity was defined recursively)
• (could be postponed) Add an ability to pass metainfo around entities. That way we can provide a way to report in error where the erroneous entity is defined, if resolver function provide that information

[dralley]

Introduce new error kind and return it when depth become 0. That means that we reach recursion limit

(could be postponed) I would like to have limit configurable

How should one configure the limit? At some point it becomes unwieldy to keep all of this state external and provide it in each method call (we also have the XML / HTML divergence in attribute handling to consider).

Should we consider keeping the state in Reader and doing something along the lines of

• reader.normalize_attribute_value(attr), or
• attr.normalize_value_with(resolve_entity, reader) or attr.normalize_value_with(reader) (moving the resolve_entity into Reader entirely)?

Also I've noticed that some implementations detect an entity loop immediately instead of processing until the recursion limit is reached. Should that be two separate errors in your opinion, or one error?

[dralley]

@Mingun ^

[Mingun]

How should one configure the limit? At some point it becomes unwieldy to keep all of this state external and provide it in each method call (we also have the XML / HTML divergence in attribute handling to consider).

Most naturally would be have a new option in reader::Config. That mean, we need somehow propagate it to the actual method. Some methods already takes Reader, so it will simple for them. Maybe we just need to start from only those methods and add shortcuts only when them explicitly will requested.

Actually, I already though about storing Decoder in the attribute itself (but because currently Attribute is a struct with public fields it will be a breaking change and I not very like the idea of making that new decoder field public, because it is implementation detail. Maybe in the end we will store already decoded data)

Also I've noticed that some implementations detect an entity loop immediately instead of processing until the recursion limit is reached.

Yes, of course we should return error as soon as we found loop or if recursion limit was exceeded.

Should that be two separate errors in your opinion, or one error?

Two different. libxml2 also have two different errors, as you could notice from your link: one is "Detected an entity reference loop", other is "Maximum entity nesting depth exceeded"

[dralley]

Obviously I haven't gotten around to finishing this

If you feel inclined to do so, I wouldn't mind if you picked it up. If not, I'll get around to it eventually, I just haven't been doing a lot of coding in my free time and what I have done, was on a different project.