Remove unused and misleading domain argument #37183
Conversation
XFormInstance.objects.iter_forms() indirectly ignored its domain argument. It was misleading to accept a domain argument because it falsely implied that only forms from that domain would be yielded.
XFormInstance.objects.get_forms() ignored its domain argument. It was misleading to accept a domain argument because it falsely implied that only forms from that domain would be returned.
CommCareCase.objects.iter_cases() indirectly ignored its domain argument. It was misleading to accept a domain argument because it falsely implied that only forms from that domain would be yielded.
CommCareCase.objects.get_cases() ignored its domain argument. It was misleading to accept a domain argument because it falsely implied that only forms from that domain would be returned.
millerdev
requested review from
AmitPhulera,
calellowitz,
esoergel,
kaapstorm,
mkangia,
snopoke and
zandre-eng
as code owners
dimagimon
added
reindex/migration
|
|
||
| def inaccessible_forms_accessed(self, xform_ids, domain, couch_user): | ||
| xforms = XFormInstance.objects.get_forms(xform_ids, domain) | ||
| xforms = XFormInstance.objects.get_forms(xform_ids) |
There was a problem hiding this comment.
A domain check seemed incorrect for what this function is doing, so I didn't add one.
| ids = case_ids | ||
| for case_id_chunk in chunked(ids, DEFAULT_CHUNK_SIZE): | ||
| case_chunk = CommCareCase.objects.get_cases(list(case_id_chunk), domain) | ||
| case_chunk = CommCareCase.objects.get_cases(list(case_id_chunk)) |
There was a problem hiding this comment.
@zandre-eng, this did not check the domain, and I think it's safe to not add an additional check here. Do you agree?
There was a problem hiding this comment.
I think that is fine. We already filter for the case_ids by domain in process_batch() so it should be safe to not have an extra check here.
| def _validate_payment_case_status(case, valid_statuses, operation): | ||
| def _validate_payment_case_status(case, domain, valid_statuses, operation): | ||
| if case.domain != domain: | ||
| raise PaymentRequestError(_("Case '{}' not in '{}' project").format(case.case_id, domain)) |
There was a problem hiding this comment.
@ajeety4, I added a domain check here as well as a new error. Does this seem right to you?
There was a problem hiding this comment.
Yes, this looks correct to me.
| def iter_documents(self, ids): | ||
| for wrapped_case in CommCareCase.objects.iter_cases(ids, self.domain): | ||
| """NOTE: unlike iter_document_ids, this does not enforce domain membership""" | ||
| for wrapped_case in CommCareCase.objects.iter_cases(ids): |
There was a problem hiding this comment.
This was the original inspiration for this PR. Pillow processors call iter_documents(ids) on a document store that has a domain, which was passed in here. That was very confusing because the ids are from many different domains, and the returned cases should not be filtered by domain.
| for case_block in case_blocks: | ||
| case = db_case_dict[case_block['@case_id']] | ||
| if case_types and case.type not in case_types: | ||
| if case.domain != domain or (case_types and case.type not in case_types): |
There was a problem hiding this comment.
@kaapstorm I had to update some tests that failed on account of this change. Do you think it's reasonable to add a domain check here?
AmitPhulera
left a comment
AmitPhulera
left a comment
There was a problem hiding this comment.
Looking good. Just one question, I see that in some places domain checks were skipped. I am sure you would have thought through the changes, just asked about the ones which I was not sure of.
| def iter_forms(self, form_ids): | ||
| """ | ||
| :param form_ids: list of form_ids. | ||
| :param domain: See the same parameter of `get_forms`. |
There was a problem hiding this comment.
nit: outdated docstring
| :param domain: See the same parameter of `get_forms`. |
|
|
||
| for case in CommCareCase.objects.iter_cases(case_ids, domain_name): | ||
| for case in CommCareCase.objects.iter_cases(case_ids): | ||
| cc_case = CallCenterCase.from_case(case) |
There was a problem hiding this comment.
Do we need to have a domain check here?
| errored_doc_ids = [] | ||
| case_ids = [rec.doc_id for rec in records] | ||
| cases = {c.case_id: c for c in CommCareCase.objects.get_cases(case_ids, session.domain)} | ||
| cases = {c.case_id: c for c in CommCareCase.objects.get_cases(case_ids)} |
There was a problem hiding this comment.
Are we sure that the records can never have any cases from other domain?
The
domainargument was removed from the following methods:XFormInstance.objects.get_forms(form_ids)XFormInstance.objects.iter_forms(form_ids)CommCareCase.objects.get_cases(case_ids)CommCareCase.objects.iter_cases(case_ids)It was misleading to accept a domain argument because it falsely implied that only forms from that domain would be returned.
Safety Assurance
Safety story
In most places this was obviously safe since the domain check was enforced nearby when fetching form or case ids. In a few it was clearly not meant to be enforced. In some cases an additional check was added because it seemed like the code wanted it, or maybe out of paranoia because the domain check was distant and/or hard to verify quickly.
In any case, the code should be more conservative about domain membership in any place where a check was added.
Automated test coverage
Some.
QA Plan
No QA.
Rollback instructions