Make deviceDisplayName optional in DeviceRegistration

[yuanchen233]

1. deviceDisplayName field is no longer required in DeviceRegistration
2. For CustomDeviceRegistration, deviceDisplayName is default to null
3. ApiVersion in DeploymentServiceRequest bumped to 1.3

Json file under deployment/src/commonTest/recourses were copied from build folder, but DeploymentServiceRequest under rpc was updated manually.

Two things to notice

1. The key-value pairs in Json request/respond are ordered. Therefore simply adding the remove field will not guarantee the request/respond to be identical to earlier api version. JsonObject in kotlinx.serailization is a Map, but the added field will by default append to the end. I added a helper function sorting the respond json with a hard-coded string list. Other approach will be appreciated.
2. Consider the following partial respond from DeploymentServiceRequest.GetDeviceDeploymentFor

"connectedDevices": [
    {
        "__type": "dk.cachet.carp.common.infrastructure.test.StubDeviceConfiguration",
        "roleName": "Connected"
    }
],
"connectedDeviceRegistrations": {
    "Connected": {
        "__type": "dk.cachet.carp.common.application.devices.DefaultDeviceRegistration",
        "registrationCreatedOn": "2022-04-04T15:03:22.340210600Z",
        "deviceDisplayName": null,
        "deviceId": "26d56d0e-2d78-4c5a-a428-bfd85d05da77"
    }
}

Connected here is the roleName, but it's used as key, which is a bit odd here as most of the keys are reserved keywords. Maybe it could be combined with connectedDevices or have its own array with roleName as a field. I'm unsure but we might also be using non-keyword keys in other request/respond, for example in SetParticipantData.

[Whathecode]

Seems like you figured most of it out! 👍

Just scrolled through it; no proper review yet. To respond to your comments:

1. I guess the problem is the test fails because it does an exact string match? Even though the ordering in JSON shouldn't matter. So, it's probably more appropriate to update the test instead; that would make a fine separate preceding commit.
2. That's simply the easiest way to serialize that map. Otherwise you need to introduce other various intermediate objects just for serialization purposes, e.g., a DeviceConfigurationWithRegistration. And, that wouldn't even take care of polymorphism yet, so you'd likely need to roll custom serializer. In normal form like this, things are simply easier. 😉

Minor comments:

• The helper function you placed in CARP common at a glance doesn't belong there. Maybe a more generic solution, decoupled from the specific deviceDisplayName (like the other helper methods) can be thought up?
• You named the upgrader "version 2 to 3", and it should be "1 to 3".

Lastly, I need to double-check what I used to do before. It seems a bit annoying all of the JSON request test resources already need to be added now. Any further changes within this release will then need to update those. But, likely that's how I did it before as well...

[yuanchen233]

1. I guess the problem is the test fails because it does an exact string match? Even though the ordering in JSON shouldn't matter.

Yes, and yes, order of object properties should not be a thing. but since we are targeting JavaScript, and apparently Most Browsers iterate object properties as: String keys, in insertion order (ES2015 guarantees this and all browsers comply), and enforced this even more later on. This will be a rare usecase for us, but send request, and get element by index from the respond would work in JavaScript...

The helper function you placed in CARP common at a glance doesn't belong there.

Yes, it would be useful everytime a field is removed. However, if we update the test and the order of JSON become irrelavent, this helper function is no longer needed.

[Whathecode]

but since we are targeting JavaScript, and apparently Most Browsers iterate object properties as: String keys, in insertion order (ES2015 guarantees this and all browsers comply), and enforced this even more later on.

Interesting! I wasn't aware about that. However, for the purpose of this discussion, I think that should be irrelevant. Our interface across systems is JSON, and not "JSON as processed by JavaScript". If we allow fields to be arranged in any order there is no point in testing that they are in the right order.

Furthermore, at a glance, kotlinx.serialization can't even be configured to require strict ordering of contained elements. The only exception, I seem to recall, is the __type property which needs to be first to make polymorphism work (don't recall this 100% certain; didn't look it up again). Consequently, the library we use also provides no guarantees in terms of ordering! So, at best, your current implementation ensuring a certain ordering depends on implementation details of the current kotlinx.serialization library. Not that we should avoid that at all costs; for unknown polymorphism I do the same, but, we'd need proper unit test coverage to make sure this still works on version upgrades.

However, long story short, I think the JSON standard correctly states ordering doesn't matter, so our upgraders shouldn't care. :)

[yuanchen233]

I looked into the unit testing and we are comparing JsonElement where order should not be relevant for equals comparison. Could simply be a mistake I made, I'll convert this into a draft and test a bit more, and also fix other things you mentioned.

[Whathecode]

Sorry. I overlooked the update to this PR. Will try to see soon whether it can be merged!

[Whathecode]

I made things a bit more concise by adding a helper function which is likely to be useful in future upgraders: 5ce4f58