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

aws_cdk.core.Stack.of() returns wrong class type (in Python) #8262

Closed
ThomasSteinbach opened this issue May 28, 2020 · 9 comments · Fixed by aws/jsii#1724
Closed

aws_cdk.core.Stack.of() returns wrong class type (in Python) #8262

ThomasSteinbach opened this issue May 28, 2020 · 9 comments · Fixed by aws/jsii#1724
Assignees
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. language/python Related to Python bindings needs-reproduction This issue needs reproduction. needs-triage This issue or PR still needs to be triaged. p1

Comments

@ThomasSteinbach
Copy link
Contributor

aws_cdk.core.Stack.of() returns wrong class type in Python

Reproduction Steps

Here is a minimal, not directly runnable (and in this context not very useful) Python code example of the behavior:

from aws_cdk import core
class MyParentStack(core.Stack):
    ...
    @classmethod
    def of(cls, construct: core.Construct) -> cls:
        stack = super().of(construct)
        print(type(stack))
        return stack

class MyChildStack(MyParentStack):
    ...

stack_parent_instance = MyParentStack(...)
stack_child_instance = MyChildStack(...)

MyParentStack.of(stack_parent_instance)
# prints: <class MyParentStack>
MyParentStack.of(stack_child_instance)
# prints: <class aws_cdk.core.Stack>

I would expect, that MyParentStack.of(stack_child_instance) would return a stack of type MyChildStack.

The behavior occurs only if the Stack.of() method is called directly on a subclass of aws_cdk.core.Stack and not on constructs within the stack itself.

Other

Yes, the example shown seems not to be very useful. But in my custom CDK library I have a 'base' stack class with custom properties and further custom stack classes derived from that base stack.

When calling the .of()-Method of my base stack on an arbitrary construct, I will ensure that the returned type of the stack is of my base stack or a subclass of it, such that I can access the custom properties.


This is 🐛 Bug Report

@ThomasSteinbach ThomasSteinbach added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 28, 2020
@michaelwiles
Copy link
Contributor

Looks like you're running foul of the reality that python is not a "first class citizen" in cdk world, only typescript. The python implementation is a mapping to typescript.

This mapping only goes one way - to typescript and does not go the other way, from typescript to python. So the Stack.of method calls the typescript equivalent and the typescript returns a stack.

But the typescript unable to return the original python type - it seems - unless that python type matches the type it was called on. I suspect that original mapping is not kept as there is no mapping from typescript to python.

I'm not sure what options you have...

If you really want it to sing the thing to do would be to code your api or library in typescript, add jsii bindings and generate the python... I think you'd get a lot of mileage if you did that.

@SomayaB SomayaB added the @aws-cdk/core Related to core CDK functionality label Jun 1, 2020
@SomayaB SomayaB added the language/python Related to Python bindings label Jun 1, 2020
@ThomasSteinbach
Copy link
Contributor Author

But the typescript unable to return the original python type - it seems - unless that python type matches the type it was called on.

This statements might not be true. As I stated above, jssi is perfectly capable to return the correct class type when the Stack.of()-method is called on

  • my custom stack class, which overrides the .of()-method
  • an arbitrary construct contained by a subclass of my custom stack class (but not the stack class itself)

As such I am confident that jsii could be able to return the proper class type even in the currently failing case, where the .of()-Method is called on the subclass of my custom stack class directly.

@eladb
Copy link
Contributor

eladb commented Jun 8, 2020

@RomainMuller @MrArnoldPalmer I was under the impression that jsii should return the actual type of the object and not the type declared on the method. Am I missing something?

@eladb eladb added the p1 label Jun 8, 2020
@RomainMuller
Copy link
Contributor

Looks like you're running foul of the reality that python is not a "first class citizen" in cdk world, only typescript. The python implementation is a mapping to typescript.

This is a gross oversimplification of what jsii is doing.

This mapping only goes one way - to typescript and does not go the other way, from typescript to python. So the Stack.of method calls the typescript equivalent and the typescript returns a stack.

This is incorrect. If an instance of some python type is passed to JavaScript, then that same instance should be what is seen from Python's point of view if that is ever read back from JavaScript. If that isomorphism does not happen for a given class instance, then there is a bug.


TL;DR: Something weird/abnormal seems to be happening here, and I'll be looking into that to understand exactly what.

@RomainMuller RomainMuller self-assigned this Jun 8, 2020
@RomainMuller
Copy link
Contributor

RomainMuller commented Jun 8, 2020

So the return type of your of override is incorrectly annotated (although I doubt this causes the problem):

   @classmethod
   def of(cls, construct: core.Construct) -> core.Stack:
      stack = super().of(construct)
      print(type(stack))
      return stack

I have tested with the following reproduction code (vertically the same as your code):

#!/usr/bin/env python3

from aws_cdk import core

class MyParentStack(core.Stack):
   ...


class MyChildStack(MyParentStack):
   ...


app = core.App()

stack_parent_instance = MyParentStack(app, 'Parent')
stack_child_instance = MyChildStack(app, 'Child')

print("Parent: " + type(core.Stack.of(stack_parent_instance)))
print("Child:  " + type(core.Stack.of(stack_child_instance)))

app.synth()

And it produces the following output:

<class '__main__.MyParentStack'>
<class '__main__.MyChildStack'>

I reckon this is conform to what you were expecting to see, so I am presently unable to reproduce your problem.

My pip list is:

Package                       Version Location
----------------------------- ------- --------------------------------------------------------
attrs                         19.3.0
aws-cdk.cdk-assets-schema     1.44.0
aws-cdk.cloud-assembly-schema 1.44.0
aws-cdk.core                  1.44.0
aws-cdk.cx-api                1.44.0
cattrs                        1.0.0
constructs                    3.0.3
jsii                          1.6.0
pip                           19.2.3
publication                   0.0.3
python-dateutil               2.8.1
setuptools                    41.2.0
six                           1.15.0
stack-of                      0.0.1   /Users/rmuller/Development/Demos/Python/StackOf/stack_of
typing-extensions             3.7.4.2

@RomainMuller RomainMuller added the needs-reproduction This issue needs reproduction. label Jun 8, 2020
@ThomasSteinbach
Copy link
Contributor Author

Thanks @RomainMuller for attending to the issue.

To be sorry, the reproduction steps from my original example did not really work - or at least did work as I would it expect from CDK.

However I could strip down my production code to a minimal example which can be run as-is and reproduces the Stack.of() behavior:

from aws_cdk import core

class MyStack(core.Stack):
    def __init__(self, scope: core.Construct, id: str, *args, **kwargs):
        super().__init__(scope, id)
        self.of(self)

    @classmethod
    def of(cls, construct: core.Construct):
        stack = super().of(construct)
        if not isinstance(stack, cls):
            raise TypeError(f"Expected {type(construct)} but got {type(stack)}")
        return stack

app = core.App()
stack = MyStack(app, "stack")
app.synth()

As you can see, the behavior has nothing to to with subclassing, as I accidentially thought first. It rather occurs if the of() method is called in the constructor of the stack class itself. As the TypeError message says, I expect the same stack type from the super().of() method as I gave as construct.

Because I stripped down the code to reproduce the behavior, there is maybe the question why I would call the of() Method within the constructor. In my production code the reasoning is as following: MyStack has properties I would like to access in other constructs. For this MyStack has a custom of() method, which does not much more than calling super().of() but also proving, that the returned stack is at least from type MyStack and thus containing the properties. For instance my custom kms key construct calls in its constructor some_prop = MyStack.of(self).property. And I have a subclass of MyStack which defines such a kms key in its constructor. Thus during the creation of the SubStack the constructor of the kms key is called which in turn is calling the MyStack.of() method during the construction of SubStack. As we have seen, the MyStack.of() method raises an error, that the returned stack of the originally given scope of type SubStack is of type aws_cdk.core.Stack.

As Python is during the construction aleady aware of the class type, I suppose that TypeScript is not and simply returns the type of the derived class. This is my first thought about the reasoning, why Stack.of() returns the wrong type.

@RomainMuller
Copy link
Contributor

I see. What could possibly go wrong here is that the reference might be read back from JavaScript before it's identifier has been bound to the current instance (possibly because it is being initialized). Consequently, a new proxy reference is initialized on reading back from JS, instead of re-using the original instance.

I must admit this is a super interesting bug... I'll dig into that today.

@RomainMuller
Copy link
Contributor

Yup - the culprit is definitely that the metaclass registers the reference into it's registry only after __init__ has returned.

RomainMuller added a commit to aws/jsii that referenced this issue Jun 9, 2020
Within the `__init__` of a class, reading `self` back from the JS
process would result in a proxy object created for the parent type,
instead of returning `self`. This is because the `JSIIMeta` metaclass
only registered the instance in the rerefence map **after** the
constructor had returned.

This adds an additional registration point right after the `create` API
returned from the kernel, making the reference available as early as it
can possibly be known.

Fixes aws/aws-cdk#8262
mergify bot pushed a commit to aws/jsii that referenced this issue Jun 9, 2020
Within the `__init__` of a class, reading `self` back from the JS
process would result in a proxy object created for the parent type,
instead of returning `self`. This is because the `JSIIMeta` metaclass
only registered the instance in the rerefence map **after** the
constructor had returned.

This adds an additional registration point right after the `create` API
returned from the kernel, making the reference available as early as it
can possibly be known.

Fixes aws/aws-cdk#8262



---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
@ThomasSteinbach
Copy link
Contributor Author

Cool, that was quick. Will the fix be part of the next jsii release? Or how do I get to know when this fix will be released?

Thanks again for the quick responses and the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. language/python Related to Python bindings needs-reproduction This issue needs reproduction. needs-triage This issue or PR still needs to be triaged. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants