Skip to content

Conversation

chasenlehara
Copy link
Member

A binding such as vm:sProperty:to is encoded to vm:s:u:property:to, which is ambiguous to the decoder; should it decode :s: first or :u: first?

Previously, we decoded :s: first, which decoded vm:s:u:property:to to vm u:property:to. Now, we decode :u: first, which should be ok because only the new bindings need to encode uppercase with :u: and the new bindings don’t support spaces in them.

Fixes #11

A binding such as `vm:sProperty:to` is encoded to `vm:s:u:property:to`, which is ambiguous to the decoder; should it decode `:s:` first or `:u:` first?

Previously, we decoded `:s:` first, which decoded `vm:s:u:property:to` to `vm u:property:to`. Now, we decode `:u:` first, which should be ok because only the new bindings need to encode uppercase with `:u:` and the new bindings don’t support spaces in them.

Fixes #11
@rjgotten
Copy link

rjgotten commented Jan 1, 2018

Imho this is just waiting for another encoding glitch to occur where someone else finds a mismatching pattern.

The only approach that is 100% predictable and correct in the general case, is to escape all existing occurences of the character that markes the escaped sequence.

I.e. escape : as ::, and logically following from that escape any occurence of ::, if it would occur, as ::::. But that will quickly become a mess with the colon also being used for qualifiers on the binding.

So maybe use a different character as the escape marker?
As Document.createAttribute follows the production rules for XML attribute names, you can use something like a middle dot \xB7 character. Low odds that anyone will ever actually include it into a binding name and even if they do: as long as you consistently escape it, you'll be fine.


Also, using a chain of String.replace calls with multiple regexes is inviting another problem with clashing escape sequences where one escaped replacement may be matched and escaped again by another further in the chain.

Generally, it is better to use one combined regex and use a replacer function. (Also performs better, iirc)

@phillipskevin
Copy link
Contributor

@rjgotten thanks for bringing up your concerns. I think we will go with this solution temporarily while we work on a more holistic solution.

@phillipskevin
Copy link
Contributor

@rjgotten I added a couple issues. Feel free to add any details / corrections on them.

@chasenlehara chasenlehara merged commit 29beb77 into master Jan 2, 2018
@chasenlehara chasenlehara deleted the 11-encoding branch January 2, 2018 17:24
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.

3 participants