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

Y24-159 - Update create-labware schema to add retention instructions #412

Open
4 tasks
dasunpubudumal opened this issue Jun 24, 2024 · 5 comments
Open
4 tasks
Assignees
Labels
Labware Destruction Size: S Small - low effort & risk Value: 4 Value to the institute is high

Comments

@dasunpubudumal
Copy link

dasunpubudumal commented Jun 24, 2024

User story
As developers, we need to add a retention_instruction field into the create-labware schema. This is to facilitate specifying retention instructions in the warehouse for each tube/plate submitted. The resulting schema should be published as a new version in RedPanda.

Who are the primary contacts for this story
Steve / Stuart / Dasun

Who is the nominated tester for UAT

Acceptance criteria
To be considered successful the solution must allow:

  • All messages received on the current message schema version are treated as implicitly specifying "Return to customer after 2 years" for the retention field.
  • All messages received on the new message schema version must contain retention_instruction field. If the message does not contain a retention instruction field (check Y24-159 - Update create-labware schema to add retention instructions #412 (comment)), a default value must be used.
  • The new version is published and stakeholders are made aware of it. The default value for the retention instruction needs to be properly communcated with the clients as this is defining how long SciOps can legally hold the samples and whether they are destroyed or returned.
  • The retention_instruction is properly documented in the schema using the doc attribute.

References
This story has a non-blocking relationship with:

Additional context
Add any other context or screenshots about the feature request here.

@dasunpubudumal
Copy link
Author

dasunpubudumal commented Jun 24, 2024

Our RedPanda schemas uses default compatibility (which is BACKWARD), which makes it not possible to add required fields. It allows deleting existing fields and/or adding optional fields.

This needs to be addressed before proceeding with the story. If the field is made optional, it will be required to have an implicit default value.

@stevieing stevieing self-assigned this Jul 10, 2024
@stevieing
Copy link

Schema changes made on schema registry for UAT.

@sdjmchattie
Copy link

Just pasting the current change here for others to see:

{
    "name": "retentionInstruction",
    "type": {
        "type": "enum",
        "name": "allowedRetentionInstruction",
        "symbols": [
            "destroy_after_2_years",
            "return_to_customer_after_2_years",
            "long_term_storage"
        ],
        "default": "destroy_after_2_years"
    },
    "doc": "The retention instruction (what the customer wants to happen to the sample)"
}

Parsing out what this means, it's an optional field called retentionInstruction and it has an enum with three values. One of the values is defined as a default in the type definition.

Questions I have @stevieing :

  • Should the default value be defined alongside the name and doc rather than inside the type?
  • Is the default value correct? I read above in this issue that it is to return to customer after 2 years.
  • Less of a question and more of an observation -- so written this way, I think the publisher must either provide one of the enum values or leave the field completely out of the message. I don't know what happens when they try to assign it with a null/None/nil value. The schema doesn't allow null as a type. Perhaps that's ideal and I haven't tested what happens to the encoded message under these conditions.

@sdjmchattie
Copy link

So I did some tests.

  • Having "default" defined at the level it is means you cannot leave this field unspecified so it does need to move out one level.
  • Not making null one of the types means exactly as I said above. You can leave the field out entirely. You can include the field as long as you give it one of the enum values. If you put the field in but assign it null, the validation fails on the message. Perhaps we want it that way. If not, we need to put null in as a type in a union.

@sdjmchattie
Copy link

I decided to try to understand what it means to have "default" in the enum itself instead of at the root of the field. It's a safety mechanism for enums specifically and only gets used when decoding a message, not during encoding. If you (somehow) received a message that gives a value for the field that isn't in the enum list, the default would be decoded instead. It's unclear how a message would be badly encoded to start with, but it could possibly happen if you are decoding with the wrong schema version, or the publisher wrote the message without using a valid Avro library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Labware Destruction Size: S Small - low effort & risk Value: 4 Value to the institute is high
Projects
None yet
Development

No branches or pull requests

3 participants