Fixes #4509 - Improve exact match search support for token parameters in matchesTokenFilter

[pamella]

Description

I opened this PR so you folks can have a closer look at the proposed partial solution mentioned in #4509.

Proposed solution

That being said, I thought about a partial solution that would allow us to support exact match searches for token parameters. I say partial because it would address the identifier and code types.

I was working on this solution, however I wonder if:

• It would be enough to have a partial solution because I understand there are a lot of nuances in FHIR standards about search such as operators and modifiers.
• It would be a overkill to implement a full solution considering (I suppose) that the main usage is for testing purposes.
• It might be a breaking change to other matches* functions that are using the matchesStringFilter function. For example, matchesAccessPolicyResourcePolicy and matchesCriteria.
• It might be a breaking change to customers that are already using the MockClient class.

So I would like to know your opinion about all of this.

Please let me know if you have any questions or need further information. I am looking forward to your feedback. Thank you!

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
medplum-provider	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 6, 2024 9:26pm

3 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
medplum-app	⬜️ Ignored (Inspect)	Visit Preview		May 6, 2024 9:26pm
medplum-storybook	⬜️ Ignored (Inspect)	Visit Preview		May 6, 2024 9:26pm
medplum-www	⬜️ Ignored (Inspect)	Visit Preview		May 6, 2024 9:26pm

[vercel]

@pamella is attempting to deploy a commit to the Medplum Team on Vercel.

A member of the Team first needs to authorize it.

[codyebberson]

Hi @pamella - thank you for submitting!

It would be enough to have a partial solution because I understand there are a lot of nuances in FHIR standards about search such as operators and modifiers.
It would be a overkill to implement a full solution considering (I suppose) that the main usage is for testing purposes.

Yes, this is an inherent challenge with MockClient. Our general philosophy has been "do as good as we can with code, but accept data limits".

This draft PR definitely qualifies for "do as good as we can with code".

The "accept data limits" disclaimer refers to the impossibility of implementing everything without 100's of MB of StructureDefinition, SearchParameter, CodeSystem, and ValueSet resources.

It might be a breaking change to other matches* functions that are using the matchesStringFilter function. For example, matchesAccessPolicyResourcePolicy and matchesCriteria.
It might be a breaking change to customers that are already using the MockClient class.

This is a fair callout. We generally accept anything that moves us closer to spec compliance, and I believe this PR qualifies for that.

Thank you for your submission. Cursory review of the code looks good to me. I'll take another deeper look once all automated tests have passed. Please let us know if you have any questions.

[pamella]

Hi @codyebberson, thank you for looking at this and sharing the context around the philosophy!

---

I noticed that a test failed but passed when I ran it locally. Would it be the case to rerun the related action job?

[pamella]

I was able to reproduce the failing test locally after rerunning npm ci and npm run build:fast. But, curiously, it also fails in the main branch. Do you folks have an initial guess as to what could be causing this?

[codyebberson]

Thanks @pamella - it looks like the test failure is consistent. We are investigating.

[pamella]

Thank you, Cody! In case there is anything I should adjust in the PR, please let me know. Meanwhile, I'm looking here to see if there's anything I can solve on my end.

[pamella]

Recent finding:

• On main branch, the test passes after running npm ci and npm run build:fast.
• On this PR branch, the test fails after running npm ci and npm run build:fast.

So indeed the changes seem to have had an impact on the test case.

If I make the following changes to the test (see lines with // Before: 'Test'), it passes, but I am not sure if that's the right move here.

➡️ Click here to expand and see the code

test('Reference filter', async () => {
  await setup({
    questionnaire: {
      resourceType: 'Questionnaire',
      status: 'active',
      id: 'reference-filter',
      item: [
        {
          linkId: 'q1',
          type: 'reference',
          text: 'Question',
          extension: [
            {
              url: 'http://hl7.org/fhir/StructureDefinition/questionnaire-referenceResource',
              valueCode: 'Observation',
            },
            {
              url: 'http://hl7.org/fhir/StructureDefinition/questionnaire-referenceFilter',
              valueString: 'subject=$subj',
            },
          ],
        },
      ],
    },
    subject: { reference: 'Patient/123' },
    onSubmit: jest.fn(),
  });

  // Add a spy on medplum.searchResources
  const searchResources = jest.spyOn(medplum, 'searchResources');

  // Get the search input
  const input = screen.getByRole('searchbox') as HTMLInputElement;

  // Enter "Simpson"
  await act(async () => {
    fireEvent.change(input, { target: { value: 'Test 1' } }); // Before: 'Test'
  });

  // Wait for the drop down
  await act(async () => {
    jest.advanceTimersByTime(1000);
  });

  expect(screen.getByText('Test 1')).toBeDefined();
  expect(searchResources).toHaveBeenCalledTimes(1);
  expect(searchResources.mock.calls[0][0]).toBe('Observation');
  expect(searchResources.mock.calls[0][1]).toBeInstanceOf(URLSearchParams);

  const params = searchResources.mock.calls[0][1] as URLSearchParams;
  expect(params.get('subject')).toBe('Patient/123');
  expect(params.get('code')).toBe('Test 1'); // Before: 'Test'
});

[codyebberson]

It looks like that test was relying on the incorrect behavior. Fix here: e9c122b

[pamella]

Thank you for the fix!

[codyebberson]

Thanks @pamella !