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

expandReferences is broken in dhis2.get (and other adaptors) #275

Closed
josephjclark opened this issue Jun 15, 2023 · 1 comment · Fixed by #280
Closed

expandReferences is broken in dhis2.get (and other adaptors) #275

josephjclark opened this issue Jun 15, 2023 · 1 comment · Fixed by #280
Assignees
Labels
bug Something isn't working

Comments

@josephjclark
Copy link
Collaborator

This is huge. And hard to explain.

If nested in an each, then dhis2.get will only resolve references on the first iteration.

So this code breaks:

each('patients[*]', get('history', (state) => ({ query: { id: state.data.id } }) )

What this SHOULD do is for each item in state.patients, resolve the query object using the reference function and fetch each patient.

But actually, it will only resolve the query object once, for the first patient, and thereafter re-use the same query object and return the same patient patients.length times.

The problem is that the get function expands references back into the closure variable, mutating it for the next call.

export function get(resourceType, query, options = {}, callback = false) {
  return state => {
    console.log('Preparing get operation...');

    resourceType = expandReferences(resourceType)(state);
    query = expandReferences(query)(state);
    options = expandReferences(options)(state);

This means that on the first invocation of get, the query is a function. It will be called and its value returned to query. So on the second invocation, the query is a string, and its value will be re-used.

Other adaptors expand to a new variable and leave the closure alone, which is correct and good:

export function get(resourceType, query, options = {}, callback = false) {
return state => {
console.log('Preparing get operation...');

const resolvedResourceType = expandReferences(resourceType)(state);
const resolvedQuery = expandReferences(query)(state);
const resolvedOptions = expandReferences(options)(state);

This problem forced @mtuchi to fall back to `fn()` and do all his iteration manually in vanilla JS.

We've identified this problematic pattern on other adaptor functions as well - they all need fixing. The fix is so significant it should cause a minor or even major version bump.
@josephjclark josephjclark added the bug Something isn't working label Jun 15, 2023
@taylordowns2000
Copy link
Member

taylordowns2000 commented Jun 15, 2023

Thanks for flagging @josephjclark ! I'll talk with @stuartc in the AM and we'll make a plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants