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

feat(sdk): adding verification expression #1332

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

igpetrov
Copy link
Contributor

@igpetrov igpetrov commented Oct 26, 2023

Description

feat(sdk): adding verification expression

@igpetrov igpetrov self-assigned this Oct 26, 2023
@igpetrov igpetrov force-pushed the verification-fields-support branch 3 times, most recently from 28bb084 to 68312ff Compare October 31, 2023 15:11
@igpetrov igpetrov changed the title [WIP] feat(sdk): adding verification expression feat(sdk): adding verification expression Oct 31, 2023
@igpetrov igpetrov marked this pull request as ready for review October 31, 2023 16:44
@igpetrov igpetrov requested a review from a team as a code owner October 31, 2023 16:44
@igpetrov igpetrov requested a review from sbuettner October 31, 2023 16:44
@igpetrov igpetrov force-pushed the verification-fields-support branch from 68312ff to d4e79e5 Compare October 31, 2023 17:24
Comment on lines +130 to +133
Optional.of(verificationResult)
.map(VerifiableWebhook.WebhookHttpVerificationResult::headers)
.ifPresent(map -> map.forEach(headers::add));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt it rather be Optional.of(verificationResult.headers())?

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 that case it would be Optional.ofNullable(...). But in that case it's the same.

protected ResponseEntity buildBodyExpressionResponseOrOk(
WebhookResult webhookResult, WebhookResultContext processVariablesContext) {
if (webhookResult.responseBodyExpression() != null) {
var httpResponseData = webhookResult.responseBodyExpression().apply(processVariablesContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldnt this throw an exception?

Copy link
Contributor Author

@igpetrov igpetrov Nov 2, 2023

Choose a reason for hiding this comment

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

Very much depends on the fucntion implementation. The method call is covered behind try-catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbuettner do you have any specific issue in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really but usually we catch exceptions where they happen.

Comment on lines 143 to 145
return ResponseEntity.ok(httpResponseData);
} else {
return ResponseEntity.ok().build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid multiple return statements.

Copy link
Contributor Author

@igpetrov igpetrov Nov 2, 2023

Choose a reason for hiding this comment

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

Reworked this and other methods. I am generally okay with multiple rets. I am wondering, why don't you like multiple return statements? We agreed on Google Java Code Style but it says nothing about returns.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its usually (not always) easier to follow but we can also discuss with the team.

@igpetrov igpetrov force-pushed the verification-fields-support branch from d4e79e5 to 529f654 Compare November 2, 2023 13:34

Verified

This commit was signed with the committer’s verified signature.
igpetrov Igor Petrov
@igpetrov igpetrov force-pushed the verification-fields-support branch from 529f654 to 3e518ca Compare November 2, 2023 13:37
@igpetrov igpetrov requested review from sbuettner and chillleader and removed request for andromaqui November 2, 2023 13:43
Copy link
Member

@chillleader chillleader left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor comment to the test case

@@ -195,4 +213,6 @@ private record TargetTypeInteger(Function<InputContextInteger, Integer> function
private record TargetTypeList(Function<InputContextInteger, List<Long>> function) {}

private record TargetTypeMap(Function<InputContextInteger, Map<String, Long>> function) {}

private record TargetTypeFoldedMap(Function<InputContextInteger, Map<String, Object>> function) {}
Copy link
Member

Choose a reason for hiding this comment

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

nit:

The original issue actually involved an Object return type that contained a map as one of its fields, right? I think it might be more representative to test the similar use case here:

private record FoldedMapRecord(Map<String, Object> folded) {}

private record TargetTypeFoldedMap(Function<InputContextInteger, FoldedMapRecord> function) {}

The reason this caught my eye, I think the current test case could have been handled by the previous implementation as well (by this code, as the root type is already a map). But I might be wrong here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spent some time trying to rewrite and didn't manage to make it working 😕
Let me merge it as-is now, and I'll follow up on this in this task.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, thank you!

@igpetrov igpetrov added this pull request to the merge queue Nov 2, 2023
Merged via the queue into main with commit 39012d7 Nov 2, 2023
1 check passed
@igpetrov igpetrov deleted the verification-fields-support branch November 2, 2023 17:26
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.

None yet

3 participants