superfaceai / one-sdk-js

1️⃣ One Node.js SDK for all the APIs you want to integrate with
https://superface.ai
MIT License
47 stars 3 forks source link

Sandbox transforms arrays to objects on the sandbox boundary #16

Closed Vratislav closed 2 years ago

Vratislav commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Maybe linked to: https://github.com/patriksimek/vm2/issues/198

Vratislav commented 3 years ago

https://github.com/patriksimek/vm2/issues/265

Vratislav commented 3 years ago

The minimalistic replication is

deals = [1,2,3]

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

Vratislav commented 3 years ago

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.

TheEdward162 commented 3 years ago

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 commented 3 years ago

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

Vratislav commented 3 years ago

I am still replicating this with:

ts-node v4.1.0
node v14.15.1
typescript v2.9.2
Vratislav commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/superfaceai/superface/pull/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Fixed by #27

lukas-valenta commented 2 years ago

This issue seems to be reintroduced in vm2: https://github.com/patriksimek/vm2/issues/198