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 converter regression #777

Closed
wants to merge 2 commits into from

Conversation

pawelprazak
Copy link
Contributor

@pawelprazak pawelprazak commented Aug 9, 2022

Description

Unit tests in separate PR: #782 on top of this one

Fixes #774

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@pawelprazak pawelprazak force-pushed the pprazak/774/fix-converter-regression branch from b3fd8e7 to 9579962 Compare August 10, 2022 07:28
@pawelprazak pawelprazak marked this pull request as ready for review August 10, 2022 07:29
));
}
setters.get(name).invoke(builder, argument);
var convertedArgument = tryConvertObjectInner(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the fix

setters.get(name).getName(),
Nullable.class.getTypeName()
));
}
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

made try...catch more narrow

@@ -396,33 +396,45 @@ private Object tryConvertObjectInner(
// call setters for all arguments
var setters = processSetters(builderType, Function.identity());
argumentsMap.forEach((name, argument) -> {
if (!setters.containsKey(name)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved from inside the try catch

TypeShape.extract(extractSetterParameter(setters.get(name)))
);
setters.get(name).invoke(builder, convertedArgument);
} catch (IllegalArgumentException e) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added info to IAE for debugging, previously got only "type mismatch error" in this case

@t0yv0 t0yv0 self-requested a review August 10, 2022 17:00
@pawelprazak pawelprazak force-pushed the pprazak/774/fix-converter-regression branch from 6201d7d to 432c858 Compare August 11, 2022 11:38
@pawelprazak
Copy link
Contributor Author

merged with #782

@pawelprazak pawelprazak deleted the pprazak/774/fix-converter-regression branch August 11, 2022 12:04
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.

Changes in packages for Java SDK
1 participant