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 all 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.