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 associated document id field to ChangeMeta #33807

Merged
merged 10 commits into from
Dec 5, 2023

Conversation

mjriley
Copy link
Contributor

@mjriley mjriley commented Nov 29, 2023

Product Description

Technical Summary

Associated ticket: https://dimagi-dev.atlassian.net/browse/SAAS-15054.
This adds an associated_document_id to all ChangeMeta objects that allows us to link case changes to the forms that triggered them. It is meant to be somewhat generic, because ChangeMeta is used by more than just cases.
Note that this PR is difficult to roll back -- while existing kafka changes will simply send a None value for the new field, if this change were to be reverted, anything with a set associated_document_id would fail to wrap into a ChangeMeta.
For this reason, I've isolated just this field change to this PR, and intend to create a separate PR for moving the duplication logic between processors.

Safety Assurance

Safety story

Verified through local testing that the additional field is added and processed correctly. Also verified that changes without the field will be processed without issue with this change. This specific change shouldn't introduce any significant overhead, as we're only slightly expanding the size of a ChangeMeta. The potential performance impact would mainly be from using the associated_document_id to fetch forms within the case processor.

Automated test coverage

Did not feel tests were needed here, but perhaps there's an argument for creating tests that verify kafka changes can still be wrapped with the new field?

QA Plan

No QA planned

Rollback instructions

Rollback of this PR would be difficult, because any changes published with this change will fail to be wrapped to the previous version of ChangeMeta. I think if we really wanted to force a revert and did not want to lose data, we'd need to create a custom wrap function for ChangeMeta that ignored the associated_document_id field.

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@mjriley mjriley added the product/invisible Change has no end-user visible impact label Nov 29, 2023
@dimagimon dimagimon added the Risk: High Change affects files that have been flagged as high risk. label Nov 29, 2023
Copy link
Contributor

@snopoke snopoke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're concerned about the rollback issue there are a few options:

  1. Set _allow_dynamic_properties to True on ChangeMeta
  2. Override ChangeMeta.wrap to cache wrapping exceptions and retry with the new key removed.

@mjriley
Copy link
Contributor Author

mjriley commented Dec 2, 2023

If you're concerned about the rollback issue there are a few options:

  1. Set _allow_dynamic_properties to True on ChangeMeta

Thanks for this. This seems like a good approach. Do we think there's a good reason to keep ChangeMeta strict? I'd imagine allowing dynamic properties would need to be done by an earlier commit so that rolling back this PR doesn't break anything

@snopoke
Copy link
Contributor

snopoke commented Dec 4, 2023

If you're concerned about the rollback issue there are a few options:

  1. Set _allow_dynamic_properties to True on ChangeMeta

Thanks for this. This seems like a good approach. Do we think there's a good reason to keep ChangeMeta strict? I'd imagine allowing dynamic properties would need to be done by an earlier commit so that rolling back this PR doesn't break anything

Yes, that's correct. I don't see a reason to keep it strict.

@mjriley
Copy link
Contributor Author

mjriley commented Dec 5, 2023

I've merged a new PR into master that lets ChangeMeta accept dynamic properties, so this PR should now be safe to roll back, if required

@mjriley mjriley merged commit 416e7f1 into master Dec 5, 2023
13 checks passed
@mjriley mjriley deleted the mjr/add-change-meta-context branch December 5, 2023 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact Risk: High Change affects files that have been flagged as high risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants