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]

There's a bug here. Or, at least, this doesn't do what the code implies. The second parameter is not additionalSpecifications, but deviceId.

And, deviceId was purposefully added as the last parameter, as in the vast majority of cases you want to not specify that yourself. Maintaining it last would have made this less likely to happen. :)

Furthermore, this method is not really common to both places where it is used. The context is different, and in this case, inlining it and making it more specific to the exact thing being tested makes more sense.

[Whathecode]

Unfortunately we don't have support for parameterized tests. At least, back when I set up this project, there was no multiplatform test framework which provided that.

But, this would be more appropriate here. Check out how I reworked this to a poor man's version of the same. :) The downside is it doesn't report individual test results, but I think that is a small price to pay for maintainability/removing code duplication.

Consider it a potentially useful investigation to look at the current status of parameterized tests in kotlin multiplatform.

[Whathecode]

Not that you could have known, but I've created makeUnknown test helpers before to help setting this up as it become a recurring thing.

[Whathecode]

This can reference ApplicationData.json for consistency with other places where this is used.

[Whathecode]

Similar to applicationData = "{\"uiTheme\": \"black\"}" and other "application data" fields for which examples are created here, let's make this a representative JSON string.

[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.