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

LambdaRestApi doesn't add the handler when .withOptions is used #965

Closed
leepa opened this issue Oct 18, 2018 · 10 comments · Fixed by aws/jsii#376
Closed

LambdaRestApi doesn't add the handler when .withOptions is used #965

leepa opened this issue Oct 18, 2018 · 10 comments · Fixed by aws/jsii#376
Assignees
Labels
bug This issue is a bug. language/java Related to Java bindings

Comments

@leepa
Copy link
Contributor

leepa commented Oct 18, 2018

If I do this - I end up with the Lambda Handler not added to the {proxy+} and instead it points to MOCK

        LambdaRestApi edgeApi = new LambdaRestApi(this, "ShopBackendEdge", LambdaRestApiProps.builder()
                .withOptions(RestApiProps.builder()
                        .withRestApiName("TroutApiEdge")
                        .withDescription("Artifishial Intelligence ordering API @ Edge")
                        .build())
                .withProxyPath("/")
                .withHandler(dummyLambda)
                .build());

If I remove the .withOptions, then handler is correctly added and the API works.

@rix0rrr rix0rrr added bug This issue is a bug. language/java Related to Java bindings labels Nov 6, 2018
@mbulman
Copy link

mbulman commented Mar 11, 2019

+1. Just discovered this an almost filed a dupe.

I think this can be traced to the constructor for LambdaRestApi where it's merging the defaultIntegration property passed in with the RestApiProps object passed into withOptions. RestApiProps extends ResourceBase which has the defaultIntegration property as well. I suspect the RestApiProps.Builder is returning a null defaultIntegration rather than not returning that key at all, which blows away the defaultIntegration explicitly set in LambdaRestApi

https://github.com/awslabs/aws-cdk/blob/707f02c3c9de58e84d5342b7f684207fcb50c8bf/packages/%40aws-cdk/aws-apigateway/lib/lambda-api.ts#L50

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 12, 2019

Oh man, that should be a jsii compliance test!

@eladb
Copy link
Contributor

eladb commented Mar 12, 2019

On it

@eladb eladb self-assigned this Mar 12, 2019
@eladb
Copy link
Contributor

eladb commented Mar 12, 2019

Oh JavaScript. TIL:

console.log({
  ...{
    hello: 'hello'
  },
  ...{
    hello: undefined
  }
});

Returns:

{ hello: undefined }

Which was not what I expected, and will make it harder to solve this.

@eladb
Copy link
Contributor

eladb commented Mar 12, 2019

The current behavior of the Java bindings is that data types (created through the generated builders) are basically normal classes that implement the jsii interface and when passed to the js code they are passed by reference and not serialized into a JSON object.

This means that when the JS code wants to read a property value (i.e. hello in the above example), it will call back to the Java code and the Java code will invoke getHello() on the PoJo, which will return null (in case the property was not specified). Even if the null is converted to undefined, it doesn't matter because the JS object has a key named hello and it will override when used like above.

@mbulman
Copy link

mbulman commented Mar 12, 2019

Sounds like an interesting general problem to solve, but for this case specifically would simply flipping order solve the issue?

Going from:

super(scope, id, {
      defaultIntegration: new LambdaIntegration(props.handler),
      ...props.options
    });

to

super(scope, id, {
      ...props.options,
      defaultIntegration: new LambdaIntegration(props.handler)
    });

props.handler is required and props.options.defaultIntegration is specifically forbidden.

@eladb
Copy link
Contributor

eladb commented Mar 12, 2019

Ideally we want to solve this at the right layer (i.e. jsii) so that user code (i.e. CDK) will not have to be aware of these cross-language quirks.

@mbulman
Copy link

mbulman commented Mar 12, 2019

Understood. But there is nothing intrinsically right or wrong about either ordering above (i.e. changing it does not make the code better or worse), so if the ideal fix is significant effort+time it would be nice to consider a short term solution for this particular case.

eladb pushed a commit to aws/jsii that referenced this issue Mar 12, 2019
Implement a serialization method for Java POJOs (produced by
calling `build()` on the generated builders such that they
are passed by-value to JavaScript instead of by-reference.

Also, erase any nulls passed in objects to JS, so they are
treated as unset values for the purpose of `key in obj`.
This fixes aws/aws-cdk#965 and fixes #375.
@eladb
Copy link
Contributor

eladb commented Mar 12, 2019

Hopefully we will be able to fix it at the source: aws/jsii#376

@mbulman
Copy link

mbulman commented Mar 12, 2019

👍

eladb pushed a commit to aws/jsii that referenced this issue Mar 12, 2019
Implement a serialization method for Java POJOs (produced by
calling `build()` on the generated builders such that they
are passed by-value to JavaScript instead of by-reference.

Also, erase any nulls passed in objects to JS, so they are
treated as unset values for the purpose of `key in obj`.
This fixes aws/aws-cdk#965 and fixes #375.
eladb pushed a commit to aws/jsii that referenced this issue Mar 20, 2019
Data types ("structs") are defined through a TypeScript interface that only contains properties and doesn't begin with an `I`. This change also adds a requirement that all properties (shallow) are marked `readonly`.

This allows passing around structs by-value safely because any consuming code would not expect to be able to write to the object.

Python already passes structs by value, especially in the context of when a struct is used as keyword arguments. This use case will also exist in Ruby.

This is also the general perception around data that's passed around in most programming languages.

To enforce this, the jsii compiler will now fail if a struct includes properties that are not marked `readonly`.

Both the Java and .NET generators have been modified to generate builders which allow users to construct structs by serializing them as JSON hashes instead of passed down by reference.

Existing compliance tests in all languages have been fixed.

This change fixes aws/aws-cdk#965 and fixes #375 by erasing any `null`s or `undefined` values from passed in objects, so they do not appear to have been defined at all. Added a compliance test called **erase-unset-data-values** to verify.

BREAKING CHANGE: all properties in interfaces which represent data types must be marked as `readonly`. Otherwise, jsii compilation will fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. language/java Related to Java bindings
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants