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

Bug with rendering #foreach and $util.map.copyAndRemoveAllKeys #140

Closed
brucek opened this issue Nov 2, 2021 · 13 comments · Fixed by #142
Closed

Bug with rendering #foreach and $util.map.copyAndRemoveAllKeys #140

brucek opened this issue Nov 2, 2021 · 13 comments · Fixed by #142

Comments

@brucek
Copy link
Contributor

brucek commented Nov 2, 2021

I am trying to use jest to test my local VTL templates

using:

  • OSX 11.6
  • node v12.18.0
  • amplify-velocity-template: 1.4.5
  • jest: 26.6.3

I just create, parse, and render a simple template with:

import * as utils from 'amplify-appsync-simulator/lib/velocity/util'
import { map } from 'amplify-appsync-simulator/lib/velocity/value-mapper/mapper'
import { Compile, parse } from 'amplify-velocity-template'

const ast = parse(`
  #set( $keyFields = ["id"] )
  #foreach( $entry in $util.map.copyAndRemoveAllKeys($ctx.args.input, $keyFields).entrySet() )

  #end
`);

const compiler = new Compile(ast, {
  valueMapper: map,
  escape: false
})
const args = {
  input: {
    id: '000',
    timestamp: '2021-12-01T08:00:00.000Z',
  }
}
const context = createVtlContext(args)
const result = JSON.parse(compiler.render(context))

function createVtlContext(args) {
  const util = utils.create([], new Date(Date.now()), Object())
  const context = {
    args,
    arguments: args
  }
  return {
    util,
    utils: util,
    ctx: context,
    context
  }
}

when I render at the last line, I get the error:

    TypeError: Cannot read property 'first_line' of undefined

      24 |     }
      25 |     const context = createVtlContext(args)
    > 26 |     const result = JSON.parse(compiler.render(context))
         |                                        ^
      27 | 
      28 |     console.log(result)
      29 | 

      at Velocity.getPropMethod (node_modules/amplify-velocity-template/src/compile/references.js:317:56)
      at Velocity.getAttributes (node_modules/amplify-velocity-template/src/compile/references.js:195:20)
      at Velocity.<anonymous> (node_modules/amplify-velocity-template/src/compile/references.js:133:24)
          at Array.some (<anonymous>)
      at Object.utils.<computed> [as some] (node_modules/amplify-velocity-template/src/utils.js:9:25)
      at Velocity.getReferences (node_modules/amplify-velocity-template/src/compile/references.js:125:15)
      at Velocity.getLiteral (node_modules/amplify-velocity-template/src/compile/literal.js:46:21)
      at Velocity.getBlockEach (node_modules/amplify-velocity-template/src/compile/blocks.js:180:24)
      at Velocity.getBlock (node_modules/amplify-velocity-template/src/compile/blocks.js:19:22)
      at Velocity.<anonymous> (node_modules/amplify-velocity-template/src/compile/compile.js:113:59)
          at Array.forEach (<anonymous>)
      at Object.utils.<computed> [as forEach] (node_modules/amplify-velocity-template/src/utils.js:9:25)
      at Velocity._render (node_modules/amplify-velocity-template/src/compile/compile.js:66:13)
      at Velocity.render (node_modules/amplify-velocity-template/src/compile/compile.js:36:22)
      at Object.<anonymous> (test/src/mapping-templates/error.test.js:26:40)

I can't figure out why or if there is something wrong with my code - I believe my code is straight from AWS Amplify codegen and does work when run as an AppSync API call on the AWS stack.

Thanks for any pointers !

@brucek
Copy link
Contributor Author

brucek commented Nov 2, 2021

FWIW I was following the instructions here to set up VTL unit testing:
https://mechanicalrock.github.io/2020/04/27/ensuring-resolvers-aren't-rejected.html

I have not yet tried another option I stumbled on:
https://github.com/skyhookadventure/appsync-template-tester

@shepherdwind
Copy link
Owner

Can you provide the basic logic code of the function createVtlContext . I need that function to run you test code.

@brucek
Copy link
Contributor Author

brucek commented Nov 3, 2021

Can you provide the basic logic code of the function createVtlContext . I need that function to run you test code.

sorry, forgot about that:

function createVtlContext(args) {
  const util = utils.create([], new Date(Date.now()), Object())
  const context = {
    args,
    arguments: args
  }
  return {
    util,
    utils: util,
    ctx: context,
    context
  }
}

@shepherdwind
Copy link
Owner

utils.create is missing.

@brucek
Copy link
Contributor Author

brucek commented Nov 3, 2021

Not sure what you mean exactly @shepherdwind:

function createVtlContext(args) {
  const util = utils.create([], new Date(Date.now()), Object()) ## calling utils.create here
  [...]

In tests with other templates (ie, try the below), parsing and rendering works fine:

const ast = parse(`
  #set( $keyFields = ["id"] )
`);

@shepherdwind
Copy link
Owner

I need the code of utils.create, I need it to run the code you provide.I need complete, repeatable code to reproduce the problem.

@brucek
Copy link
Contributor Author

brucek commented Nov 5, 2021

I need the code of utils.create

Sorry; my misunderstanding. I am using utils from the AWS amplify-appsync-simulator

As I mentioned, I am just following the code template from https://mechanicalrock.github.io/2020/04/27/ensuring-resolvers-aren't-rejected.html

Thanks for your patience 🙏

@brucek
Copy link
Contributor Author

brucek commented Nov 6, 2021

@shepherdwind Random Q: what is this line doing?

if (typeof index === 'number') {

as far as I can tell, this will never be true, and so the first branch will never execute

Also, the current test suite never executes it as well

Not sure if related to this issue, but just discovered it while investigating

brucek added a commit to brucek/velocity.js that referenced this issue Nov 6, 2021
shepherdwind added a commit that referenced this issue Nov 8, 2021
Addressing #140, also added a bit more defensive code in case pos is missing in error situations
@shepherdwind
Copy link
Owner

@shepherdwind Random Q: what is this line doing?

if (typeof index === 'number') {

as far as I can tell, this will never be true, and so the first branch will never execute

Also, the current test suite never executes it as well

Not sure if related to this issue, but just discovered it while investigating

The code was submitted by others #124 , I did not look closely at the specific implementation at the time, and just tried it does not run to this branch.

Also,Did the #141 pr fix the bug you mentioned here with that commit? I'll publish a new version directly, and then I will find time to refactor this repository code later, it's a bit too old.

@shepherdwind
Copy link
Owner

I am sorry I don't know about AWS amplify-appsync-simulator very much, I have never touched this tool before . A pr with test code would be more useful, and it would be perfect if it included test pass code.

@brucek brucek changed the title Bug with rendering #foreach and $util.map.copyAndRemoveAllKeys Bug with rendering #foreach and having the iterated collection returned from a function call Nov 8, 2021
@brucek brucek changed the title Bug with rendering #foreach and having the iterated collection returned from a function call Bug with rendering #foreach and $util.map.copyAndRemoveAllKeys Nov 8, 2021
@brucek
Copy link
Contributor Author

brucek commented Nov 8, 2021

So the #141 pr fixed a related issue which is that the error output was itself erroring and gave unusable output.

I'm still not yet sure what is the root cause of the original issue

Possibly related to this? serverless-appsync/serverless-appsync-simulator#106

@shepherdwind
Copy link
Owner

shepherdwind commented Nov 9, 2021

I know what the problem is, I optimized the exception stack display, and now the exceptions look like this.

     TypeError: map.keySet is not a function on $util.map.copyAndRemoveAllKeys($ctx.args.input,$keyFields).entrySet() at L/N 3:6
      at Object.copyAndRemoveAllKeys (node_modules/_amplify-appsync-simulator@1.27.7@amplify-appsync-simulator/lib/velocity/util/map-utils.js:34:14)
      at Velocity.getPropMethod (src/compile/references.js:315:23)
      at Velocity.getAttributes (src/compile/references.js:188:20)

And the code of amplify-appsync-simulator/lib/velocity/util/map-utils.js:34:14 like this

    copyAndRemoveAllKeys: function (map, keys) {
        var keysStr = keys.toJSON();
        var result = map
            .keySet()
            .toJSON()

So the root cause of this issue is the map do not have the keySet methods. I publish a new beta version, so you can try the beta version on your local machine.

There are two ways to solve this problem, first wrap the input to an map, for example

const args = {
input: mapper.map({
id: '000',
timestamp: '2021-12-01T08:00:00.000Z',
})
}

The second one, maybe the complier option valueMapper should map every value, now the valueMapper only handle #set condiction.

@shepherdwind
Copy link
Owner

new version 2.0.5 published

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.

2 participants