Skip to content
This repository has been archived by the owner on Aug 19, 2022. It is now read-only.

Implement json.Marshaler interface for LimitConfig #67

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

MarcoPolo
Copy link
Contributor

@MarcoPolo MarcoPolo commented Jul 6, 2022

Fixes a bug where we would output the json with the raw peer id rather than the encoded version. This would mean would aren't able to round trip serialize the config. e.g. take the config, write it json, read it from json and compare it with the original config.

This uses the type alias marshaling trick. More can info about this can be found at https://calvinfeng.gitbook.io/gonotebook/idioms/custom-json-marshaling#better-approach (and many other sources).

@marten-seemann
Copy link
Contributor

I‘m wondering why we don’t have a JAON marshaling function for the peer.ID. That would solve this issue, wouldn’t it?

@MarcoPolo
Copy link
Contributor Author

I‘m wondering why we don’t have a JAON marshaling function for the peer.ID. That would solve this issue, wouldn’t it?

This wouldn't solve this issue. (In fact we are already doing JSON marshaling for peer.ID src).

It doesn't solve this issue because the way Go json encodes a map:

// Map values encode as JSON objects. The map's key type must either be a
// string, an integer type, or implement encoding.TextMarshaler. The map keys
// are sorted and used as JSON object keys by applying the following rules,
// subject to the UTF-8 coercion described for string values above:
//   - keys of any string type are used directly
//   - encoding.TextMarshalers are marshaled
//   - integer keys are converted to strings
//

source and impl.

In this case peer.ID is a string type (playground) so it gets used directly.

We have a couple options:

  1. Do this in the type that contains a map[peer.ID].... Which is this change.
  2. Introduce a new type that wraps map[peer.ID].... Similar to this change but bigger.
  3. Replace or introduce a new type that uses a struct instead of string. e.g. type ID struct { id string }. (probably a fairly big breaking change)

I chose 1 here because it's the smallest change that doesn't introduce any breaking changes.

Does that make sense or did I miss something?

@marten-seemann
Copy link
Contributor

This wouldn't solve this issue. (In fact we are already doing JSON marshaling for peer.ID src).

Sorry, I was on my phone, and didn't look it up before.

I do think we need to make a bigger change to the peer.ID at some point, but this is not the time to do it. This PR looks like the right approach in this situation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants