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

Sandbox transforms arrays to objects on the sandbox boundary #16

Closed
Vratislav opened this issue Nov 17, 2020 · 17 comments · Fixed by #232
Closed

Sandbox transforms arrays to objects on the sandbox boundary #16

Vratislav opened this issue Nov 17, 2020 · 17 comments · Fixed by #232
Assignees

Comments

@Vratislav
Copy link
Contributor

Behavior

given profile snippet

usecase GetActivities {
  input {
    filterId
    userId
  }

  result {
    activities
  }
}

and given map snippet

map GetActivities {
  http GET "/api/v1/activities" {
        security apikey query {
          name = "api_token"
        }   
    request {
          query {
               filter_id = input.filterId
               user_id = input.userId
          }       
    }

    response 200 {

        map result if (body.success) {
          activities = body.data
        }

        map error if (!body.success) {}
      }
  }
}

result.value.activities contains Dictionary instead of Array (Array indicies are used as keys)

Expected behaviour

I expected result.value.activities to contain array

(But maybe I am wrong and the map implicitly dictates conversion to Dictionary in this case?)

@lukas-valenta
Copy link
Contributor

@Vratislav I don't think this is a Superface issue, from the map it seems that nothing is done with the body.data, did you look at the response if it's a problem there? if not, could you try Array.isArray on activities? arrays actually are objects in javascript..

@Vratislav
Copy link
Contributor Author

I do not know, where exactly the problem is, but the behaviour is unexpected.

If I receive body.data and data is an array (confirmed by postman) and then I do activities = body.data.map((d) => ...) on it, the result is a Map with IDs as a keys. :-/

@Vratislav
Copy link
Contributor Author

Huh, that's strange. I can replicate with doing this in the map:
activities = [1,2,3].map(d => d)
Output is:
{ '0': 1, '1': 2, '2': 3 }

@Vratislav
Copy link
Contributor Author

Maybe linked to: patriksimek/vm2#198

@Vratislav
Copy link
Contributor Author

patriksimek/vm2#265

@Vratislav
Copy link
Contributor Author

The minimalistic replication is

deals = [1,2,3]

Output is:
{ '0': 1, '1': 2, '2': 3 }

@Vratislav
Copy link
Contributor Author

I am just thinking whether JSON.stringify and then JSON.parse on the result after the sandbox boundary would be a good workaround or not.

@Vratislav Vratislav changed the title HTTP Arrays is translated to Dictionary without reason Sandbox transforms arrays to objects on the sandbox boundary Nov 21, 2020
@TheEdward162
Copy link
Contributor

Can you please specify your node --version?

I managed to reproduce [ 1, 2, 3, '0': 1, '1': 2, '2': 3 ] on v10.23.0 but jest still claims toStrictEqual([1, 2, 3]) and Array.isArray also returns true.

On node v12.19.1, v14.15.1 and v15.2.0 I cannot reproduce this.

@Vratislav
Copy link
Contributor Author

My node is v12.4.0. I will try to update to the latest LTS

@Vratislav
Copy link
Contributor Author

I am still replicating this with:

ts-node v4.1.0
node v14.15.1
typescript v2.9.2

@Vratislav
Copy link
Contributor Author

But i can only replicate it as part of map walkthrough. If I do a test on the sandbox itself, such as:

  it('passes array unchanged', () => {
    expect(evalScript('[1,2,3]')).toStrictEqual([1, 2, 3]);
  });

I cannot replicate it either

@Vratislav
Copy link
Contributor Author

Cannot it be the

  async visitAssignmentNode(node: AssignmentNode): Promise<NonPrimitive> {
    return this.constructObject(node.key, await this.visit(node.value));
  }

in the map-interpreter.ts ?

It seems to me that everything it touches, it objectifies.

@TheEdward162
Copy link
Contributor

I've created a branch and a PR where I added the debug package and logging calls to some interesting places in map-interpreter and Sandbox here #18.

You can see the debug output by setting environment variable DEBUG='superface:*' before executing the code.

I also added a test which should test your theory with constructObject but sadly that doesn't seem to be it.

@TheEdward162
Copy link
Contributor

I am still replicating this with:

ts-node v4.1.0
node v14.15.1
typescript v2.9.2

It looks like your ts-node and typescript versions are pretty old. Can you try upgrading those as well? If it's a dependency version bug it'd be great if we could find where it starts.

@lukas-valenta
Copy link
Contributor

i still can't reproduce, try the version upgrade (superface needs typescript > 4, i'm surprised it works at all). it definitely seems linked to the vm though

@lukas-valenta
Copy link
Contributor

Fixed by #27

@lukas-valenta
Copy link
Contributor

This issue seems to be reintroduced in vm2: patriksimek/vm2#198

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants