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(python): correctly handle structs out of callbacks #1009

Merged
merged 14 commits into from
Nov 28, 2019
Merged

Conversation

RomainMuller
Copy link
Contributor

@RomainMuller RomainMuller commented Nov 19, 2019

When moving out of callbacks, the result was not turned into a reference
before being passed out to the jsii kernel, causing TypeErrors when
attempting to turn structs (and probably other non-primitive types) to
JSON.

Discovered while attempting to reproduce #1003, which can no longer
trigger as it depended on returning a Struct through and any and being
hit with the bug fixed in #997... the fix also removed the conditions
that make leveraging the cause possible (or at the very least I could
not come up with a reproduction, and have verified that the original bug
report no longer reproduces once #997 is in).

That being said, still introduced the necessary changes to address the
cause by properly discovering interfaces and adding the relevant
overrides filtering, as this may improve the performance and reliability
of the jsii runtime for Java.

Fixes #1003
Fixes #1005
Fixes #1007
Fixes #1035


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

@RomainMuller RomainMuller requested review from bmaizels and a team as code owners November 19, 2019 10:18
@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

When moving out of callbacks, the result was not turned into a reference
before being passed out to the jsii kernel, causing TypeErrors when
attempting to turn structs (and probably other non-primitive types) to
JSON.

Discovered while attempting to reproduce #1003, which can no longer
trigger as it depended on returning a Struct through and `any` and being
hit with the bug fixed in #997... the fix also removed the conditions
that make leveraging the cause possible (or at the very least I could
not come up with a reproduction, and have verified that the original bug
report no longer reproduces once #997 is in).

That being said, still introduced the necessary changes to address the
cause by properly discovering interfaces and adding the relevant
overrides filtering, as this may improve the performance and reliability
of the jsii runtime for Java.

Fixes #1003
@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 added the pr/do-not-merge This PR should not be merged at this time. label Nov 19, 2019
@RomainMuller
Copy link
Contributor Author

Parking this PR for now as it may not be necessary to get the Java issues out of the way (as noted, they are already rendered impossible by other changes pending release).

@RomainMuller RomainMuller removed the request for review from bmaizels November 19, 2019 10:52
@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

@RomainMuller RomainMuller removed the pr/do-not-merge This PR should not be merged at this time. label Nov 26, 2019
@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 aws deleted a comment from aws-cdk-automation Nov 27, 2019
@aws aws deleted a comment from aws-cdk-automation Nov 27, 2019
@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

@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

@mergify
Copy link
Contributor

mergify bot commented Nov 28, 2019

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot merged commit 812d8c2 into master Nov 28, 2019
@mergify mergify bot deleted the rmuller/fix/1003 branch November 28, 2019 08:57
@mergify mergify bot removed the ready-to-merge label Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment