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

fix(kernel): revert behavior change around any serialization #932

Merged
merged 13 commits into from
Nov 5, 2019

Conversation

RomainMuller
Copy link
Contributor

The change of behavior in any serialization introduced in #825 risks
breaking some emerging use-cases of the AWS CDK that are likely to be
prevalent in the installed user base. As a consequence, surgically
reverting this particular change in order to restore the behavior that
users might be dependent on.

This somehow uncovered an outstanding bug in the Python runtime that
was not triggering with the "reverted" behavior (it broke unit tests).
The runtime was fixed in order to address this bug.


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

The change of behavior in `any` serialization introduced in #825 risks
breaking some emerging use-cases of the AWS CDK that are likely to be
prevalent in the installed user base. As a consequence, surgically
reverting this particular change in order to restore the behavior that
users might be dependent on.

This somehow uncovered an outstanding bug in the Python runtime that
was not triggering with the "reverted" behavior (it broke unit tests).
The runtime was fixed in order to address this bug.
@RomainMuller RomainMuller requested review from garnaat and a team as code owners November 5, 2019 09:08
@RomainMuller RomainMuller added the pr/do-not-merge This PR should not be merged at this time. label Nov 5, 2019
@RomainMuller
Copy link
Contributor Author

Labelled with do-not-merge waiting for me to create a CDK build using this new version of jsii to double-check it achieves the desired result.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…assed to callbacks

In addition, fix a bug where Python's static invoke
did not handle Callback results.
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

This fixes callback object deserialization for publicly exposed classes.
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like some unrelated changes got done. But as long as the tests still pass, I suppose everything should still be good...

// NOTE: Not entirely sure yet whether this is a bug masquerading as a
// feature or not.
const prevRef = objectReference(value);
if (prevRef) { return prevRef; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brave that you're taking this out. Seems unrelated though, so why risk it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, it was causing a bug because this code path would not add new interface annotations to an existing object reference like it should (an object I already passed out as "instance of X" and am now passing out as "implementor of Y"). The fix would have required duplicating the code a couple of lines below - which actually achieves the same result idempotently. This is the why :)

@RomainMuller RomainMuller removed the pr/do-not-merge This PR should not be merged at this time. label Nov 5, 2019
@RomainMuller RomainMuller removed the request for review from garnaat November 5, 2019 16:33
@RomainMuller RomainMuller removed the request for review from bmaizels November 5, 2019 16:37
@mergify
Copy link
Contributor

mergify bot commented Nov 5, 2019

The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged.
[Conventional Commits]: https://www.conventionalcommits.org

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@RomainMuller RomainMuller merged commit 2f47543 into master Nov 5, 2019
@RomainMuller RomainMuller deleted the rmuller/revert-serialization-change branch November 5, 2019 17:05
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 this pull request may close these issues.

3 participants