Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add custom participant attribute API backwards compatibilty test #453

Merged
merged 1 commit into from
Mar 2, 2024

Conversation

Whathecode
Copy link
Member

@Whathecode Whathecode commented Dec 10, 2023

Currently, there was no coverage for adding custom participant attributes to protocols in the backwards compatibility tests. The test resource also serves as an example on how such a JSON request looks like, which was a request in #451 .

Example of a protocol with both a default attribute (the server is expected to have the type information available at compile time), and a custom attributes (the type information is provided by the client in the form of an "input element").

{
    "id": "0d57f923-955e-4693-a606-cca9651aab4a",
    "createdOn": "2023-12-10T18:51:39.511538600Z",
    "version": 0,
    "ownerId": "27879e75-ccc1-4866-9ab3-4ece1b735052",
    "name": "Test protocol",
    "description": "Test description",
    "expectedParticipantData": [
        {
            "attribute": {
                "__type": "dk.cachet.carp.common.application.users.ParticipantAttribute.DefaultParticipantAttribute",
                "inputDataType": "dk.cachet.carp.input.sex"
            }
        },
        {
            "attribute": {
                "__type": "dk.cachet.carp.common.application.users.ParticipantAttribute.CustomParticipantAttribute",
                "input": {
                    "__type": "dk.cachet.carp.common.application.data.input.elements.Text",
                    "prompt": "Social Security Number"
                },
                "inputDataType": "dk.cachet.carp.input.custom.a1eda4bcd0d84ca0ba446cabfa4e84c8"
            }
        },
        {
            "attribute": {
                "__type": "dk.cachet.carp.common.application.users.ParticipantAttribute.CustomParticipantAttribute",
                "input": {
                    "__type": "dk.cachet.carp.common.application.data.input.elements.SelectOne",
                    "prompt": "Favorite OS",
                    "options": [
                        "Windows",
                        "Linux",
                        "Mac"
                    ]
                },
                "inputDataType": "dk.cachet.carp.input.custom.4ee5a0cc0ce5462b94baee4e59415f9b"
            }
        }
    ]
}

The protocol service does not validate inputDataType. Only when setting the participant data on ParticipationService, errors will occur if no type information is available for the provided inputDataType. In the Kotlin APIs, the inputDataType for custom participant attributes is generated under the namespace dk.cachet.carp.input.custom with a GUID appended, but, this is currently no requirement for the API.

@Whathecode Whathecode requested a review from bardram December 10, 2023 18:40
@Whathecode Whathecode force-pushed the custom-participant-data-example branch from c03ecea to 2f470c7 Compare December 10, 2023 18:52
@bardram
Copy link
Collaborator

bardram commented Dec 15, 2023

LGTM. Only comment / questions is whether it is necessary to use UUID for the custom inputDataType, like

"inputDataType": "dk.cachet.carp.input.custom.4ee5a0cc0ce5462b94baee4e59415f9b"

Can't I call them something useful, like

"inputDataType": "dk.cachet.carp.input.custom.OperatingSystem"

@bardram
Copy link
Collaborator

bardram commented Dec 15, 2023

Another question is - when do you use lower versus upper case - what is the difference between these two?

  "attribute": {
                "__type": "dk.cachet.carp.common.application.users.ParticipantAttribute.DefaultParticipantAttribute",
                "inputDataType": "dk.cachet.carp.input.sex"
            }

Why is sex lower case and DefaultParticipantAttribute upper case?

@bardram bardram self-assigned this Dec 15, 2023
bardram

This comment was marked as duplicate.

@bardram
Copy link
Collaborator

bardram commented Dec 15, 2023

Maybe it would also be a good idea to add an example of how to upload this custom participant data to the ParticipationService? Would you, for example, need to know these strange UUIDs?

@Whathecode
Copy link
Member Author

Whathecode commented Dec 15, 2023

Can't I call them something useful, like ...

You can, but as mentioned elsewhere, I recommend you to not use CARP core namespaces to rule out any potential conflicts, and just to be clear about id scopes. That said, the .custom namespace is pretty safe as by design the goal is to only add guids there

The reason they are generated by default is because when using an API exposed through a UI, you wouldn't have to directly interact with any ID. It is much more meaningful to display the prompt the user provided to help them identify which is which. In the API, you hold a reference to the object after deserializing, so no ID is needed either. That said, I am not opposed to an API which optionally allows you to set a custom ID.

Another question is - when do you use lower versus upper case - what is the difference between these two?

That's unfortunately something that leaks out from the implementation. The PascalCase types are types in the codebase using default polymorphic serialization, which just uses the namespace.ClassName. The lowercase types are data types, which are handled differently and registered manually, since they are so central to the framework. I think it would make sense that at least all extendable types (data, devices, tasks) adopt this lowercase style, but I see that is not the case atm (devices use PascalCase).

Maybe it would also be a good idea to add an example of how to upload this custom participant data to the ParticipationService? Would you, for example, need to know these strange UUIDs

Yes, but you receive these through the endpoint which tells you what data can be set. Visually, you would use the prompt to discern which is which, but for hardcoded programming scenarios, a known id would indeed be useful. I think there is an example of setting a default attribute id, but I think the operation is the same for custom attributes. Although, you may need to know how to use the 'custom input' data type; that may be missing!

I'll see whether I can include an example in this PR.

@Whathecode
Copy link
Member Author

Yes, but you receive these through the endpoint which tells you what data can be set.

I just noticed that although you do receive expected input elements on PrimaryDeviceDeployment through the expectedParticipantData property, this doesn't help a web interface as part of study management to display these elements.

When you receive known types and you have access to the CARP runtime, you can retrieve the statically defined input types. But, for custom types, you'd need access to the original protocol to discern the input elements (or derived types through which input elements are passed, such as PrimaryDeviceDeployment). This is undesirable coupling.

Incorporating support for retrieving InputElement's as part of ParticipationService will make sense. Maybe they can optionally be requested as part of getParticipantData. Or, a new endpoint to retrieve input elements could be added. I'll give this some thought. Both seem backwards compatible.

Currently, there was no coverage for adding custom participant attributes to protocols in the backwards compatibility tests. The test resource also serves as an example on how such a JSON request looks like, which was a request in #451.
@Whathecode
Copy link
Member Author

I moved the relevant tasks which emerged from this discussion to new issues, and will go ahead and merge this PR.

@Whathecode Whathecode merged commit bdf4186 into develop Mar 2, 2024
3 checks passed
@Whathecode Whathecode deleted the custom-participant-data-example branch March 2, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants