Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: enable handling of nested fields when injecting request_option in request body_json #201
base: main
Are you sure you want to change the base?
feat: enable handling of nested fields when injecting request_option in request body_json #201
Changes from 17 commits
f2d644f
00d2036
9c98313
a524f3e
fcaf110
fd2de58
20cc5d6
7292b57
ebb1791
5051d0d
fef4a46
9525357
6638dc8
eba438b
5dc29ca
6fc897f
97cff3f
7191128
4ef2423
d1fde99
adec90b
6e8e13c
0fabdd7
fdfa92c
a2262a9
e36a06a
b8287d6
fd919b1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we simplify the logic by having in post_init:
This way, we would only have one logic to maintain and it would be the
field_path
one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit skeptical that this could potentially lead to conflicts in the Builder if we started mixing the attributes, but I think I see what you're saying. If we just modify the internal logic here in the CDK to allow
field_name
, but only use it to update and usefield_path
instead, we simplify the backend handling while still allowing the Builder to define them as entirely separate fields, correct?My only hangup is wondering if there isn't still some scenario where by allowing
field_path
to essentially overridefield_name
, we could accidentally send those updated values back to the frontend? And it would be picked up as a manifest change? I don't yet know enough about the full lifecycle of the back and forth between the CDK and Builder components to know if that's a valid concern.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the case, we haven't made a good job at designing our interfaces and we will lack the benefits of easy maintenance. In an ideal world, we should be able to replace the whole CDK implementation by something else and the Connector Builder should continue to work.
Yes! Instead of knowing that there are two cases, we all rely on the cases where we handle a list and that's it!
My understand is that the only case we send back manifest information to the frontend is when we do
resolve_manifest
(or something like that) and it feels like this flow only work with the manifest directly and not with Python implementations so I would expect that we would be fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would precise that it is "If there only one mapping that is a string, it will return this mapping regardless of the other mappings".
That being said, I don't understand this behavior though. Maybe I'm wrong but before, I think it would fail if there was a string and a mapping, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like the following logic is a bit complex. Would there be a way to simplify this? Right now, I see that we:
Would it be simpler to do just the last one and validate for conflicts while we do that? Something like:
If we are afraid of modifying the mappings in memory, we can create a deepcopy of it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've modified the logic to be more in line with your suggestion. I removed the flattening and rebuilding of data in favour of a single helper method that recursively merges mappings directly, so it should hopefully be cleaner. Updated the tests as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: A number of unit tests went red due to the updated mapping logic being more permissive in allowing duplicate keys to exist. I made some more changes to handle
body_json
injections differently than other request types. So:For header, parameter, and body_data requests:
For body_json requests only:
This distinction is controlled by an
allow_same_value_merge
flag incombine_mappings
which is only set toTrue
forbody_json
requests. But I'm wondering if this is leading to the logic being overly contrived again. Another option could be to create two separate methods for handling either scenario?