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

CDK for .NET does not use the compiler to flag construct required properties #1845

Closed
mabead opened this issue Feb 24, 2019 · 6 comments
Closed
Assignees
Labels
bug This issue is a bug. effort/small Small work item – less than a day of effort language/dotnet Related to .NET bindings p0

Comments

@mabead
Copy link

mabead commented Feb 24, 2019

I am using the CDK version 0.24.1 with c#. If I create a stack that contains this:

new Amazon.CDK.AWS.Lambda.Function(this, "SomeId", new FunctionProps());

It compiles perfectly. But then, when I run cdk diff, I get the following error:

Unhandled Exception: Amazon.JSII.Runtime.JsiiException: Cannot read property 'name' of undefined
   at Amazon.JSII.Runtime.Services.Client.TryDeserialize[TResponse](String responseJson)
   at Amazon.JSII.Runtime.Services.Client.ReceiveResponse[TResponse]()
   at Amazon.JSII.Runtime.Services.Client.TryDeserialize[TResponse](String responseJson)
   at Amazon.JSII.Runtime.Services.Client.ReceiveResponse[TResponse]()
   at Amazon.JSII.Runtime.Services.Client.TryDeserialize[TResponse](String responseJson)
   at Amazon.JSII.Runtime.Services.Client.ReceiveResponse[TResponse]()
   at Amazon.JSII.Runtime.Services.Client.TryDeserialize[TResponse](String responseJson)
   at Amazon.JSII.Runtime.Services.Client.ReceiveResponse[TResponse]()
   at Amazon.JSII.Runtime.Services.Client.TryDeserialize[TResponse](String responseJson)
   at Amazon.JSII.Runtime.Services.Client.ReceiveResponse[TResponse]()
   at Amazon.JSII.Runtime.Services.Client.TryDeserialize[TResponse](String responseJson)
   at Amazon.JSII.Runtime.Services.Client.ReceiveResponse[TResponse]()
   at Amazon.JSII.Runtime.Services.Client.TryDeserialize[TResponse](String responseJson)
   at Amazon.JSII.Runtime.Services.Client.ReceiveResponse[TResponse]()
   at Amazon.JSII.Runtime.Services.Client.TryDeserialize[TResponse](String responseJson)
   at Amazon.JSII.Runtime.Services.Client.ReceiveResponse[TResponse]()
   at Amazon.JSII.Runtime.Services.Client.TryDeserialize[TResponse](String responseJson)
   at Amazon.JSII.Runtime.Services.Client.ReceiveResponse[TResponse]()
   at Amazon.JSII.Runtime.Services.Client.TryDeserialize[TResponse](String responseJson)
   at Amazon.JSII.Runtime.Services.Client.ReceiveResponse[TResponse]()
   at Amazon.JSII.Runtime.Deputy.DeputyBase..ctor(DeputyProps props)

So two problems here:

  1. The error message does not say anything useful. It is pretty hard to know which construct is causing the error.
  2. The error is caused by the fact that I didn't provide the required FunctionProps properties to my lambda. The required function properties are handler, code and runtime.

I'd like to focus on the second point. The fact that we get a runtime error and not a compile time error is a big lost compared to TypeScript. In TypeScript, if I don't provide a required property, I see it directly in my IDE:

image

Given that C# is a compiled language, I would expect that all required properties be enforced at compile time.

In my opinion, the way to do this is simply to use constructor for the different props. For example, the FunctionProps is currently defined like this:

public class FunctionProps : DeputyBase, IFunctionProps
{
   // The class just has a default/empty constructor
   public FunctionProps() {}
   // These 3 properties are required but there is no way to differentiate 
   // them from optional properties from the compiler point of view.
   public string Handler { get; set; }
   public Code Code { get; set; }
   public Runtime Runtime { get; set; }
   // All other properties are optional
   public string FunctionName { get; set; }
}

My suggestion would be to use a constructor with parameters instead of relying on getters/setters. Ex:

public class FunctionProps : DeputyBase, IFunctionProps
{
    public FunctionProps(
        // Required parameters come first and don't have default values.
        string handler,
        Code code,
        Runtime runtime,
        // Optional parameters come second and *DO* have default values
        string functionName = null)
    {
        Handler = handler;
        Code = code;
        Runtime = runtime;
        FunctionName = functionName;
    }

    // Required properties
    public string Handler { get; set; }
    public Code Code { get; set; }
    public Runtime Runtime { get; set; }
    // Optional properties
    public string FunctionName { get; set; }
}

With such code, I get a nice intellisense that tells me what is required and what is optional:

image

And if I instantiate a FunctionProps and don't provide the required properties, I get a compile time error:

error CS7036: There is no argument given that corresponds to the required formal parameter 'handler' of 'FunctionProps.FunctionProps(string, Code, Runtime, string)'

And finally here's how it looks like when I define a lambda.

new Amazon.CDK.AWS.Lambda.Function(this, "SomeId", new FunctionProps(
    handler: "my handler",
    code: Code.Inline("my code"),
    runtime: Runtime.NodeJS810,
    // Optional parameters below: since they have default 
    // values in the constructor, I can omit specifying them.
    //
    // I use a named parameter here but I could also have used 
    // the property setter. It's just a matter of preference.
    // Personally, I prefer to use named constructor parameters 
    // for the optional parameters since they end up at the same 
    // indentation level as the required parameters.
    functionName: "SomeName");

Please let me know what you think of this proposition.

@mabead
Copy link
Author

mabead commented Feb 24, 2019

Oh, and having a nicer error message in the console would be appreciated as well😉

@eladb eladb added the language/dotnet Related to .NET bindings label Feb 24, 2019
@eladb
Copy link
Contributor

eladb commented Feb 24, 2019

@costleya what do you think?

@mabead
Copy link
Author

mabead commented Mar 8, 2019

@costleya what do you think of this proposition? Are all the props data structure code generated from some meta data that says if a property is required or optional?

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 20, 2019

My take on this is that we should get rid of the props objects and replace them with keyword arguments. @eladb ?

@costleya
Copy link
Contributor

My take is to replace them with keyword arguments. We can definitely do this when we start doing some work on the generator. We'll try to schedule to do this work sometime in April or May.

@fulghum fulghum added the p0 label Apr 2, 2019
@fulghum fulghum added the effort/small Small work item – less than a day of effort label Apr 22, 2019
@assyadh
Copy link
Contributor

assyadh commented Aug 5, 2019

Related to aws/jsii#210

Adding the roslyn analyser should fix this.

@SomayaB SomayaB added the bug This issue is a bug. label Nov 11, 2019
RomainMuller added a commit that referenced this issue Nov 13, 2019
RomainMuller added a commit that referenced this issue Nov 13, 2019
1. Upgrades `jsii` to `0.20.5`(fixes #4989, fixes #4966, fixes #1904, fixes #1845)
2. Fixes the `ecs-patterns` library bug #4983
3. Fixes the CLI's "legacy" mode of operation bug #4998
@eladb eladb assigned RomainMuller and unassigned costleya Nov 13, 2019
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. effort/small Small work item – less than a day of effort language/dotnet Related to .NET bindings p0
Projects
None yet
Development

No branches or pull requests

8 participants