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

ignore property values that are not strings #30488

Closed
wants to merge 1 commit into from

Conversation

calellowitz
Copy link
Contributor

@calellowitz calellowitz commented Sep 29, 2021

Technical Summary

It is possible for forms to have certain errors that cause HQ to save case property updates in non-string formats (specifically a dictionary in this case). This changes the case search index to not index (but still include in the saved doc) case properties whose values are not strings. This is the same thing we do with overly long case properties, and for the date and numeric versions of string values. Currently these docs just hard fail on attempting to save in the case search index

Tagged everyone who has case search ES context or form processor context, although since the properties are still available in the doc, just not searchable, I don't think this really commits us to any specific action with how we handle the form processing part of this issue.

More details in https://dimagi-dev.atlassian.net/browse/SAAS-12687 with the example cases, the forms that created them, and how that happened, as well as some context from a brief conversation I had with Clayton.

Feature Flag

Case Search and Case List Explorer

Safety Assurance

  • Risk label is set correctly
  • All migrations are backwards compatible and won't block deploy
  • The set of people pinged as reviewers is appropriate for the level of risk of the change
  • If QA is part of the safety story, the "Awaiting QA" label is used
  • I have confidence that this PR will not introduce a regression for the reasons below

Safety story

I will test this on staging, but we use the same option in other parts of the mapping file, and this is broadly equivalent to #30009 for a different set of issues.

Rollback instructions

To rollback this change, the PR must be reverted and then cchq <env> django-manage update_es_mapping case_search must be run on all environments on which it was deployed.

@dimagimon dimagimon added reindex/migration Reindex or migration will be required during or before deploy Risk: High Change affects files that have been flagged as high risk. labels Sep 29, 2021
@calellowitz calellowitz added product/invisible Change has no end-user visible impact and removed reindex/migration Reindex or migration will be required during or before deploy labels Sep 29, 2021
@dimagimon dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Sep 29, 2021
@calellowitz calellowitz force-pushed the ce/non-string-property-values branch from ce51890 to 630f7af Compare September 29, 2021 15:46
@calellowitz
Copy link
Contributor Author

Turns out this setting does not work for text fields, and even where it works, it does not work for json values (which is what the broken value is here), so trying an alternate approach. The restrictions are only documented for the current version of ES, but do apply as far back as ours https://www.elastic.co/guide/en/elasticsearch/reference/7.15/ignore-malformed.html

There is an open ticket to address these issues, but it has been open 6 years, so no real optimism about quick resolution. elastic/elasticsearch#12366

@calellowitz calellowitz deleted the ce/non-string-property-values branch September 29, 2021 18:41
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 reindex/migration Reindex or migration will be required during or before deploy 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.

3 participants