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

Fix awsProvider.js : "Cannot use 'in' operator to search for '0' #5688

Merged
merged 1 commit into from Jan 17, 2019

Conversation

Projects
5 participants
@exoego
Copy link
Contributor

exoego commented Jan 12, 2019

What did you implement:

Closes #5681.

How did you implement it:

Same handler warnings introduced in #5638 does not care about ${variable} syntax.
This PR moves the warning code to after variable population.

This is a bit more complex than my initial thought.

// populate function names after variables are loaded in case functions were externalized
// (https://github.com/serverless/serverless/issues/2997)
this.service.setFunctionNames(this.processedInput.options);
// validate the service configuration, now that variables are loaded
this.service.validate();

Function names, those to be warned regarding same handler, is populated in L149 of Serverless.js.
However, service name validation is performed in Variables#prepopulateService which is invoked before function name population.
So, if functions is not an array but a string (variable syntax), it causes the error.

So, same-handler warning should be moved to Service#validate which is invoked after function name population.

How can we verify it:

  1. npm install -g exoego/serverless#fix-5681

  2. Run sls deploy against below. Verify deploy is successfull and same handler warnings are shown.

service:
  name: fix5681
provider:
  name: aws
  runtime: nodejs8.10 
functions: ${self:custom.lambdaFunctions}
custom:
  lambdaFunctions:
    functionA:
      handler: handler.hello
    functionB:
      handler: handler.hello
  1. Run sls deploy against below. Verify deploy is aborted by stage name validation error.
service:
  name: fix5681
provider:
  name: aws
  stage: inv@lid # NOTE: Invalid stage name for API Gateway
  runtime: nodejs8.10 
functions: ${self:custom.lambdaFunctions}
custom:
  lambdaFunctions:
    functionA:
      handler: handler.hello
    functionB:
      handler: handler.hello

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@exoego exoego force-pushed the exoego:fix-5681 branch from fff0cf2 to 5d30fec Jan 12, 2019

Show resolved Hide resolved lib/classes/Service.js Outdated

@pmuens pmuens added the pr/in-review label Jan 15, 2019

@horike37

This comment has been minimized.

Copy link
Member

horike37 commented Jan 15, 2019

@exoego
Seems like this still failing with the following error. Does this work fine on your end?

$ serverless deploy
 
  Serverless Error ---------------------------------------
 
  Serverless plugin "/Users/horike/.nodebrew/node/v8.12.0/lib/node_modules/serverless/lib/plugins/aws/provider/awsProvider.js" initialization errored: Cannot use 'in' operator to search for '0' in ${self:custom.lambdaFunctions}
 
  Get Support --------------------------------------------
     Docs:          docs.serverless.com
     Bugs:          github.com/serverless/serverless/issues
     Issues:        forum.serverless.com
 
  Your Environment Information -----------------------------
     OS:                     darwin
     Node Version:           8.12.0
     Serverless Version:     1.36.0

@exoego exoego changed the title Move same handler warning to after variable population. WIP: Move same handler warning to after variable population. Jan 16, 2019

Move stage name validation to Service#validate.
Since validation should be performed after service.functions are resolved.

@exoego exoego force-pushed the exoego:fix-5681 branch from 628571d to f434009 Jan 16, 2019

@exoego exoego changed the title WIP: Move same handler warning to after variable population. Fix awsProvider.js : "Cannot use 'in' operator to search for '0' Jan 16, 2019

@exoego

This comment has been minimized.

Copy link
Contributor Author

exoego commented Jan 16, 2019

@horike37
Sorry, the previous change does not fix the issue.
PR is updated and I believe it fixes the issue now.

@@ -763,6 +763,141 @@ describe('Service', () => {
expect(consoleLogStub.callCount).to.equal(2);
});
});

describe('stage name validation', () => {
function simulateRun(serverless) {

This comment has been minimized.

@exoego

exoego Jan 16, 2019

Author Contributor

Note: simulateRun is a simplified emulation of Serverless#run.
This is a bit tricky but required to populate function names, I think.

@pmuens pmuens assigned pmuens and unassigned pmuens Jan 16, 2019

@horike37
Copy link
Member

horike37 left a comment

@exoego
Thank you for updating on this 👍 Woks fine as expected again! and left one comment for sync usage. Please check it.

const provider = this.serverless.getProvider('aws');
if (provider) {
const stage = provider.getStage();
this.getAllFunctions().forEach(funcName => {

This comment has been minimized.

@horike37

horike37 Jan 16, 2019

Member

Is it possible to use BbPromise.each function instead of forEach ? We're currently enforcing the policy that all new code should be as async as possible since sync calls are on of the roots of the sloweness of the Framework.

This comment has been minimized.

@exoego

exoego Jan 17, 2019

Author Contributor

Yes, it can be async technically.
But I think it is premature optimization and not necessary for this case.

Both Service#getAllFunctions and Service#getAllEventsInFunction used in validation are just iterating over functions (and its events).
I assume number of those are several dozens in most cases, and thousands in very extreme situation.
I believe Node.js is fast enough for such number of elements.

And, other validations in Service#validate are also iterating over functions synchronousely.
I can make them all asynchronously, but it requires changes a lot in existing tests.
I think it is better to optimize in another PR when users complains.

This comment has been minimized.

@horike37

horike37 Jan 17, 2019

Member

@exoego
OK 👍 Thank you for considering it 😄

@horike37 horike37 merged commit c3da897 into serverless:master Jan 17, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 90.863%
Details

@exoego exoego deleted the exoego:fix-5681 branch Jan 17, 2019

@eahefnawy eahefnawy added this to In progress in Serverless via automation Jan 17, 2019

@eahefnawy eahefnawy moved this from In progress to Reviewer approved in Serverless Jan 17, 2019

@eahefnawy eahefnawy moved this from Reviewer approved to Done in Serverless Jan 17, 2019

@eahefnawy eahefnawy added this to the v1.36.2 milestone Jan 17, 2019

@eahefnawy eahefnawy added pr/accepted and removed pr/in-review labels Jan 17, 2019

@shortjared shortjared added the bug label Jan 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment