Impl oaschema hashmap

[thorjelly]

Implements OaSchema on HashMap to represent openapi v3 dictionaries. Openapi v3 dictionaries only support string keys. serde_json will error if the key cannot serialize into a string, but the error is given at runtime and I am unsure what appropriate compile time bound to use to enforce this, so I have left the key unbounded. We probably also want to support BTreeMap, but we could add that to a separate PR.

Depends on PR #4

[kurtbuilds]

Good point on this. I think for K, two ideas:

• bound could be K: Display
• don't have it generic at all, and only provide it for HashMap<String, V>

I'm actually leaning towards the latter, since it should cover 99% of cases, and it's easier to expand functionality rather than take it away if we realize later that we shouldn't provide e.g. HashMap<i32, V>. What do you think?

[thorjelly]

Hello, sorry, this time I was on vacation the past week this time.

I am skeptical of the claim that HashMap<String, V> is appropriate in 99% of cases as I already, in fact almost exclusively, use my keys as newtypes of i32. It is very useful. I imagine the most common use of a key would be an ID, either a number or a Uuid.

Furthermore, the problem with making a bound on K: Display or, more generally, K: ToString, is that this is not actually how Serde works. Serde will still give the runtime error "key must be a string" on K even if K: ToString if Serialize cannot serialize it into a string. As far as I am aware, Serialize does not fall back to ToString if the appropriate serialization method returns an error. So, this would require us to implement our own serializer on HashMap<K: ToString, V>, which explicitly calls to_string() on K. And this is not a good solution because the type still needs to be deserialized from the string. Also this probably runs into orphan rule (though this can be worked around).

My feelings right now are that this is not a schema problem, but a serde problem, and by allowing serde to make a runtime error as it is designed to do, it is being dealt with in a serde-like way. Someone could, for example, if they wanted to, create their own serializer/deserializer for K and it would work fine, and I don't see a reason to not allow them to. Serde gives an appropriate, understandable runtime error if the serializer cannot serialize the type, and serde already understands that the key of a hashmap must be a string, so it does that check. What we lose is a compile time check; what we gain is fully using serde and all ease of implementation and flexibility that entails.

As a side note, with Paperclip I've noticed it does, in fact, require a K: ToString bound, however as far as I am aware this bound is pretty useless for the aforementioned reasons and all it did is create useless boilerplate and it does not prevent runtime errors. It is a tempting bound, because the openapi spec requires keys to be strings, however practically speaking it does nothing because ToString is not used in the serialization process.

[kurtbuilds]

Ok. That's a great write up and explanation. I'll give this another review later this week, but expect to merge in as is (along with other PRs) later this week.

Your PRs have all been outstanding, by the way. Incredible work!

[thorjelly]

Thanks, I appreciate it a lot! And I really appreciate you working with me on these changes.