Adding output template to read requests #5054
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
Passing run #8886 ↗︎
Details:
Review all test suite changes for PR #5054 ↗︎ |
||||||||||||||||||||
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5054 +/- ##
==========================================
+ Coverage 86.42% 86.50% +0.08%
==========================================
Files 356 353 -3
Lines 22181 22176 -5
Branches 2913 2920 +7
==========================================
+ Hits 19169 19184 +15
+ Misses 2497 2478 -19
+ Partials 515 514 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
i like this new feature a lot @galvana! i think the implementation looks solid, just a few nits and then a question about whether it would be wise to have some more formal guard rails around the output template value.
i'll approve tentatively though and leave that to your discretion 👍
| @@ -194,8 +194,8 @@ def validate_grouped_inputs(cls, values: Dict[str, Any]) -> Dict[str, Any]: | |||
|
|
|||
| return values | |||
|
|
|||
| @root_validator | |||
| def validate_override(cls, values: Dict[str, Any]) -> Dict[str, Any]: | |||
| @root_validator(allow_reuse=True) | |||
There was a problem hiding this comment.
nit: does this really need the allow_reuse=True? i thought this was used if you actually wanted to reuse the same function/method for multiple validators; in your case here, you're defining a new validate_request validator function in the ReadSaaSRequest now below, right?
There was a problem hiding this comment.
You're right, I wanted the validator on ReadSaaSRequest to override the validator on SaaSRequest but @root_validator is enough, I don't need allow_reuse
| # This allows us to build an output object even if we didn't generate and execute | ||
| # any HTTP requests. This is useful if we just want to select specific input_data | ||
| # values to provide as row data to the mask_data function | ||
| if not read_request.path and read_request.output: |
There was a problem hiding this comment.
nit: this may read a bit simpler and i think is equivalent?
| if not read_request.path and read_request.output: | |
| elif read_request.output: |
| if processed_row: | ||
| param_value_map[ALL_OBJECT_FIELDS] = processed_row | ||
| row = assign_placeholders(output_template, param_value_map) | ||
| result.append(json.loads(row)) # type: ignore |
There was a problem hiding this comment.
is this fully insured that it's valid, de-serializable JSON here? do we perhaps need some validation on the output field for the read request? (i'll leave a comment up there too)
| that is used to format each collection result. | ||
| """ | ||
|
|
||
| output: Optional[str] |
There was a problem hiding this comment.
per comment below, just wondering if we should have some sort of validation here that this is a valid JSON template?
There was a problem hiding this comment.
This won't necessarily be valid JSON until the placeholders are evaluated, I'll just log out the invalid JSON further downstream if there are any parsing issues
galvana
added
the
run unsafe ci checks
Passing run #8891 ↗︎
Details:
Review all test suite changes for PR #5054 ↗︎ |
||||||||||||||||||||
Closes PROD-2209
Description Of Changes
Introduces the concept of "output templates" for
readSaaSRequests. This allows the user to specify a template that will be used to generate each row of a collection response. This was done to allow param values (connector params, identity values, dataset reference values) to be aggregated in areadrequest and be used as input data forupdate/deleterequests.To demonstrate the usefulness of this new concept, I refactored the following integrations to use output templates instead of request overrides:
Code Changes
ReadSaaSRequestschema to only allow theoutputfield forreadrequestsSaaSConnectorclass to support output templatesSaaSQueryConfigSaaSRequestandReadSaaSRequestschemasSaaSConnector.retrieve_data()Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md