Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add marshalling for constraints #167

Merged
merged 1 commit into from Nov 28, 2022
Merged

Conversation

SimonTheLeg
Copy link
Contributor

This PR adds marshal and unmarshal for constraints (see #130). I tried to built it similar to the already existing implementations for the version type, but there were some challenges

Challenges

json.Marshal escapes certain characters
Unlike version, constraints can also contain '<' and '>' characters. Unfortunately these get escaped when using json.Marshal directly (note the escapeHTML option in source). As a result we cannot simply call json.Marshal inside our custom MarshalJSON. Therefore I have decided to use a custom encoder that disables HTML escaping.

So far so good. Unfortunately our troubles do not end there. As a matter of fact encoding.json calls additional logic after it has called our custom Marshaler. In our case, the troublesome part is the compact func. The main issue here is that it runs its own escaper, which we cannot overwrite. As a result if you are marshalling using json.Marshal, it will always contain the unicode runes for them (e.g. "\u003c" for "<").

Implications for end-users

I think for most users this should be fine, as you can use the escaped characters directly within semVer. For example you can use them in the NewConstraint func without any issues:

s := "\u003c1.1.1"
cs, err := NewConstraint(s)
if err != nil {
t.Error(err)
}
fmt.Println(cs) // will print <1.1.1

And if that is not enough, users can always use a custom Encoder with SetEscapeHTML disabled instead of json.Marshal. This is similar to how we do it in the unit test:

buf := new(bytes.Buffer)
enc := json.NewEncoder(buf)
enc.SetEscapeHTML(false)
err = enc.Encode(cs)
...
// buf.String() will have nothing escaped

Open Questions

  • Should we add some more test-cases to TestNewConstraint which contain escaped characters?
  • I noticed in the Unmarshal part of Version, that each field is copied separately. Is there an advantage of this versus directly deepcopying?

@mattfarina mattfarina merged commit 00300c4 into Masterminds:master Nov 28, 2022
@mattfarina
Copy link
Member

@SimonTheLeg I'm fixing a problem I just discovered in it. The top level json encoder is the one that needs the html escaping disabled. What happens down in the individual marshaler does not impact < and > being escaped.

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.

None yet

2 participants