semver icon indicating copy to clipboard operation
semver copied to clipboard

add marshalling for constraints

Open SimonTheLeg opened this issue 3 years ago • 0 comments

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?

SimonTheLeg avatar Dec 21 '21 17:12 SimonTheLeg