feat(map): add serialisation for map<string, string>

[jonathan-casey]

This change adds basic serialisation of a map in the form of <string, string>. Further type variations will follow in subsequent PR.

For more information on the design specification see here

Screenshots or Video

Related Issues

• Issue Record type to express key/value pairs #447

Author Checklist

• Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
• Vital features and changes captured in unit and/or integration tests
• Commits messages follow AP format
• Extend the documentation, if necessary
• Merging to main from fork:branchname

[dselman]

Some questions:

1. Don't we want to support Map values other than String?
2. Runtime type for maps, support JS object?

[dselman]

Why do we need to loosen this?

[jonathan-casey]

MapDeclaration doesn't implement isTransaction()

[mttrbrts]

Perhaps it should? we could move the function to the Declaration class?

[dselman]

Values don't have to be strings though... won't this generate invalid maps? I think you want to return a map of N entries, with random string keys (use the same method we use to generate random strings) and then random values, based on the type of the value — again reusing the existing logic.

[jonathan-casey]

Yes, but for now we are supporting <String, String> only. In future the method will generate random values of all types supported.

[dselman]

Is the grammar for CTO aligned with this?

[dselman]

Do we not want to accept simple JS objects, indexed as Maps? I think this will force users to create Maps in their code rather than just JS objects.

[dselman]

We only support values that are Strings?

[dselman]

Map values have to be Strings?

[jonathan-casey]

As per comment below: The alignment on the first release was <String, String> support only.

[jonathan-casey]

Some questions:

1. Don't we want to support Map values other than String?
2. Runtime type for maps, support JS object?

1. Yes, but in a future phase. The alignment on the first release was <String, String> support only. (The rest is coming, I promise!)
2. As per spec: Maps should be implemented using the JavaScript [Map object](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map). We inherit the semantics of that implementation here.

[DS-AdamMilazzo]

@jonathan-casey @mttrbrts @dselman I think it's very unfortunate (and ugly) that the map is pushed down into a "value" property. It seems that the only reason is so that you can put Concerto metadata like $class at the top level of the map without colliding with user keys. However, I'd argue that there is virtually no reason to ever put such metadata there.

If you don't intend to allow a map to be extended, e.g. map Foo { ... } map Bar extends Foo { ... }, then there is no need for a $class, because the type will always equal the declared type of the field. (Furthermore, there is no need for $timestamp, $identifier, etc. because those are for concepts and have no meaning for a map.)

Let's say you were considering map extension. I'd argue it's a bad idea in most cases. There has to be a relation between the keys and values of the base and derived maps. In particular, the derived keys and values could either be a subset or a superset of the base keys and values. In this case, where they're a superset...

concept B { }
concept D extends B { }
concept D2 extends B { }
map M {
  o String
  o D
}
map M2 extends M {
  o String
  o B
}

This violates the basic expectations of polymorphism, because you can store a B or D2 in M2, thereby violating the requirements of the base map M. This should just never be allowed, from the standpoint of a type system.

In the other case, where they're a subset...

concept B { }
concept D extends B { }
concept D2 extends B { }
map M {
  o String
  o B
}
map M2 extends M {
  o String
  o D
}

First of all, you can already store a D (or D2) as a value in map M via basic polymorphism on B, with no need to extend map M to make that happen. Second, it's backwards from how extends would work for concepts and enums, where the derived type is a superset of the base type. Third, it violates a basic expectation of polymorphism, that a derived type can be treated as a base type. If you have an instance of map M2, you can't store a B or D2 in it. But if you treat M2 as an instance of M, which it is, then you can store a B or D2 in it. That seems kind of broken, conceptually. Do you really want to allow this?

It's not completely crazy, though. It can make some sense if we analogize to scalars. Let's say there was a way to extend a scalar type:

scalar PhoneNumber extends String regex=/^[0-9](-[0-9]+)*$/
scalar USPhoneNumber extends PhoneNumber regex=/^([1-9][0-9]{2}-){2}[0-9]{4}$/

A derived scalar would be a subset of the base scalar, just as a scalar now is a subset of the base type (e.g. String), and just as, in this case, a derived map is a subset of a base map. The important question, though, is how do you know when to apply the PhoneNumber rules versus the USPhoneNumber rules? Strings have no $class to tell you whether a string is a PhoneNumber or a USPhoneNumber. So how could you do it? It would have to be done based on the declared field type. That is to say, you have extension without polymorphism.

And if maps are extended analogously to scalars (i.e. they are a restriction of the base type, not an expansion), and if map polymorphism violates some expectations of polymorphism, then if we allow map extension at all (which I'd argue is less obviously useful than scalar extension with its layered constraints), it should also be extension without polymorphism. And without polymorphism, you need no $class property.

Let's look at an example of map extension with polymorphism anyway. Let's say you have concept C { o M map } and this instance data:

{
  "$class": "ns.C",
  "map": {
    "key": { "$class": "ns.D" }
  }
}

• Would you need a $class property to read the data? No. All derived maps are instances of the base map type, M, which we know from the field definition. A derived map can't define additional "fields" like a derived concept can.
• Would you need a $class property to write the data? If you're writing valid data, then no, but if you're writing invalid data, then yes.
• Do you need a $class property to validate the data? Yes, in order to apply the added type constraint from the derived map. However, it is exactly analogous to this situation: concept C { o PhoneNumber phone }, where you want to send a USPhoneNumber and have it validated as such. How would you do that? Maybe just change the field type to USPhoneNumber. Or, we could support type restrictions in derived types, like concept C2 extends C { o USPhoneNumber phone override }. But I'd argue that whatever the solution is for scalars, the same solution should be used for maps. In that case, no $class is needed.

So to summarize:

• Maps don't need $class unless they support both extension and polymorphism.
• Map extension only makes sense as a restriction of the base map, analogous to extension of scalars.
• Map extension may be less useful than scalar extension.
• Map extension would have as much need for polymorphism as scalar extension, i.e. none.
• Map extension also violates some expectations of polymorphism and so shouldn't have it anyway.

Overall, the case for needing $class in a map is extremely thin. So can we please move the map data out of the "value" property? This:

{
  "foo": "bar"
}

looks so much nicer than:

{
  "$class": "[email protected]",
  "value": {
    "foo": "bar"
  }
}

[jonathan-casey]

@DS-AdamMilazzo thanks for your feedback. I agree with you on the $class value remarks, as do some others, and ultimately the communities voice takes precedent and the change has followed in a subsequent PR.

In future it would be most helpful to share your opinion before a sign off. The spec was public and shared for several months prior to implementation. In any case, your feedback is always welcome and appreciated.

[DS-AdamMilazzo]

I gladly would have, but these things don't always come to my attention.