Add additionalSpecifications field to DeviceRegistration

[yuanchen233]

As discussed in #431, adding a additionalSpecifications field to DeviceRegistration. This allows to keep additional specification data when register device and access them later on in StudyDeploymentStatus.deviceStatusList

[yuanchen233]

For DeviceRegistrationTest, original test cases were kept to cover situations when additionalSpecifications is null; same for GenerateExampleRequests for rpc, additionalSpecifications is only added to one of the deviceRegistration.

[Whathecode]

See comments; I already fixed those (you can resolve them after reading them).

In addition, I squashed three of the commits into one. Each commit ideally should pass all tests/be a cohesive whole. The last commit was a correct separate commit. 👍 I hope/presume you didn't have other changes on this branch, so I force pushed. Apologies if you did (as that can cause conflicts). Make sure to use git pull --rebase.

But, I see two other concerns before this can be merged:

• I don't recall 100% whether or not we need @Required on the field. I vaguely remember it may be needed for some scenario, maybe "unknown polymorphic serialization". I need to dig up some old issues/documentation.
• You didn't test whether additionalSpecification actually gets serialized as a JSON object (instead as a string with escaped \" chars, etc.). I put a breakpoint and apparently it doesn't. I suspect that is because the serializer used for that field is not "inherited" as you would expect it. I need to dig in to kotlinx serialization documentation to refresh my memory.

[Whathecode]

Regarding @Required, I found something relevant here in context of a fix I applied.

Missing @Required attributes on fields which should always be serialized but had values assigned by initializers. These fields are needed since the backend does not necessarily have the concrete types available, and these fields are hence not optional. Applying @Required solves that.

That makes sense for fields like DeviceRegistration.deviceId, and definitely PrimaryDeviceConfiguration.isPrimaryDevice, but it seems less relevant for something like additionalSpecifications, which I don't expect to ever have type-specific defaults.

Now I'm actually wondering whether DeviceRegistration.deviceDisplayName even ever needed @Required. It didn't seem like I considered/questioned that in the related PR. I'm thinking not applying @Required (as you did here) is correct, and adding it for deviceDisplayName may have been erroneous due to a naive mimicking of surrounding code.

And, regarding the ApplicationDataSerializer not kicking in, my intuition seems right:

I suspect that is because the serializer used for that field is not "inherited" as you would expect it.

For this to work as expected, you would have to apply @Serializable( ApplicationDataSerializer::class ) to all extending classes of DeviceRegistration. Try applying it to DefaultDeviceRegistration which is used in the test, and see the difference in the serialized output.

@Serializable
@JsExport
data class DefaultDeviceRegistration(
    @Required
    override val deviceDisplayName: String? = null,
    @Serializable( ApplicationDataSerializer::class )
    override val additionalSpecifications: String? = null,
    @Required
    override val deviceId: String = UUID.randomUUID().toString(),
) : DeviceRegistration()

This gives ... "additionalSpecifications":{"OS":"Android 42"} ... as expected, instead of the erroneous ... "additionalSpecifications":"{\"OS\":\"Android 42\"}" ...

That would be a bit unfortunate if there is no way around this. It would be worthwhile checking the kotlinx serialization documentation to see whether they have any (new) features which can avoid the need for doing this.

[yuanchen233]

In addition, I squashed three of the commits into one. Each commit ideally should pass all tests/be a cohesive whole. The last commit was a correct separate commit.

Got it, I was expecting a 'squash and merge' when this branch is done, but saw a TODO warning when pushing. I didn't give much thought if those changes should be applied here after having fun reading that serialization issue

Now I'm actually wondering whether DeviceRegistration.deviceDisplayName even ever needed @Required. It didn't seem like I considered/questioned that in the related PR. I'm thinking not applying @Required (as you did here) is correct, and adding it for deviceDisplayName may have been erroneous due to a naive mimicking of surrounding code.

You mentioned in this PR deviceDisplayName was initially not optional, then the @Required annotation makes sense there, initially at least. I tested with and with out the annotation, both passes all the tests we currently have.

Regarding removing @Required from .deviceDisplayName, My concern is backward compatibility, especially JS target. As Defaults are not encoded, default null value will become undefined.

And, regarding the ApplicationDataSerializer not kicking in, my intuition seems right:
I suspect that is because the serializer used for that field is not "inherited" as you would expect it.

For this to work as expected, you would have to apply @Serializable( ApplicationDataSerializer::class ) to all extending classes of DeviceRegistration.
...
That would be a bit unfortunate if there is no way around this. It would be worthwhile checking the kotlinx serialization documentation to see whether they have any (new) features which can avoid the need for doing this.

This actually makes sense, as how kotlinx.serialization works with generic type parameters/abstract class, a serializer is required for each type/subclass. Note we also have @Serializale annotation for each subclass of DeviceRegistration. If @Serializable(with=..) for a field can be inherited, I would expect subclass also inherite @Serializable form superclass (which is not the case).

One work around I can think of is creating a new datatype/class to handle ApplicationData instead of using String, then we can set and enforce serializer for that class. The annotation will then only needed once when we declare the class, and ApplicationDataSerializer will be used as the default serializer, instead of String.serializer().

[Whathecode]

One work around I can think of is creating a new datatype/class to handle ApplicationData instead of using String, then we can set and enforce serializer for that class. The annotation will then only needed once when we declare the class, and ApplicationDataSerializer will be used as the default serializer, instead of String.serializer().

Good idea!

I think I considered it in the past. Maybe I didn't pursue it since without inheritance there was no clear benefit. I shortly looked for past considerations, but couldn't find any, only the PR where it was introduced.

So, definitely worthwhile giving this a go! You may want to do that as a separate PR preceding this one. But, also fine as separate commit within this one. Whichever you prefer. :) I won't force push anymore without checking in with you; the branch is yours.

[yuanchen233]

Regarding removing @Required from .deviceDisplayName, My concern is backward compatibility, especially JS target. As Defaults are not encoded, default null value will become undefined.

I ran some tests and it is the case:

carp.deployments.core/src/commonTest/resources/test-requests/DeploymentService/1.0/DeploymentServiceTest/
createStudyDeployment_registers_preregistered_devices.json
Request #3 returned the wrong response. ==> expected: 

<{"deviceConfiguration":{...},"registration":{"__type":"dk.cachet.carp.common.application.devices.DefaultDeviceRegistration","registrationCreatedOn":"2022-04-04T15:03:22.341209Z","deviceDisplayName":null,"deviceId":"17124c6e-c8e0-457e-9b6b-f66860c4b666"},"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"}}}> 
but was: 
<{"deviceConfiguration":{...},"registration":{"__type":"dk.cachet.carp.common.application.devices.DefaultDeviceRegistration","registrationCreatedOn":"2022-04-04T15:03:22.341209Z","deviceId":"17124c6e-c8e0-457e-9b6b-f66860c4b666"},"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","deviceId":"26d56d0e-2d78-4c5a-a428-bfd85d05da77"}}}>

"deviceDisplayName":null is missing due to kotlinx serializer not generating default values. If we want to change this, I think it's best to create another issue and plan to fix it for the next major release version? As far as I remember we don't support backward compatibility cross major versions.

[Whathecode]

That can easily be resolved with a version migration, so only a minor version bump is needed.

But, definitely out of scope here. 👍