Multivalue metafield macro support #99
Conversation
There was a problem hiding this comment.
@fivetran-avinash thanks for these updates! Please see my comments and suggestions below.
I did end up going back on my idea to remove the id field. After a deeper investigation into the macro and the use case, I feel it would be best to explicitly define the id for each macro call.
macros/get_metafields.sql
Outdated
| {{ return(adapter.dispatch('get_metafields', 'shopify')( | ||
| source_object=source_object, | ||
| reference_values=reference_values, | ||
| lookup_object=lookup_object, | ||
| key_field=key_field, | ||
| key_value=key_value, | ||
| reference_field=reference_field | ||
| )) }} | ||
| {%- endmacro %} |
There was a problem hiding this comment.
I believe we aligned in the past that the adapter.dispatch shouldn't include the default values as that can result in inconsistent results. Can you remove these default values.
There was a problem hiding this comment.
Updated. I can file a feature request to make similar updates to the recent Netsuite macro I created as well.
There was a problem hiding this comment.
That would be great, just to stay on top of ensuring our standards are consistent across the board.
macros/get_metafields.sql
Outdated
| {% endif %} | ||
|
|
||
| {# Derive the _id column dynamically #} | ||
| {% set id_column = reference_values[0] ~ '_id' %} |
There was a problem hiding this comment.
This is a unique approach! However, this makes me question future maintainability. Future contributors will be required to know that the first value in the list is what drives the _id and that isn't documented clearly anywhere. This is more a reflection on the initial design as it's a bit of a forced shortcut than a more robust design (speaking as the one who initially designed this 😆).
I know we discussed removing the id parameter, but seeing the nuances behind this macro, it seems to be the best design approach to ensure we clarify the id_column for each reference. This way it's extremely explicit and we're aware of these changes clearly moving forward.
This would apply to all the metafield macro calls and would not have a default value. Each metafield call would need to input the id column.
There was a problem hiding this comment.
Good points about future maintenance. Simple code > gnarly code.
Updated.
There was a problem hiding this comment.
@fivetran-joemarkiewicz Changes addressed and ready for re-review.
There was a problem hiding this comment.
@fivetran-avinash just one small comment for you to look at. Other than that, this looks good to go!
macros/get_metafields.sql
Outdated
| @@ -37,8 +53,8 @@ final as ( | |||
| {% endfor %} | |||
| from source_table | |||
| left join lookup_object | |||
| on lookup_object.{{ reference_field }}_id = source_table.{{ reference_value }}_id | |||
| and lookup_object.{{ reference_field }} = '{{ reference_value }}' | |||
| on lookup_object.owner_resource_id = source_table.{{ id_column }} | |||
There was a problem hiding this comment.
I actually think we should keep the old approach for the owner_resource_id where it was {{ reference_field }}_id. I imagine it was parameterized for a reason and we should probably maintain that instead of hard coding owner_resource_id.
Let me know if you think differently.
There was a problem hiding this comment.
Agreed, applied!
PR Overview
This PR will address the following Issue/Feature: Internal Height ticket
This PR will result in the following new package version: 0.16.1
This is an update of a macro to catch new field values from an API update but does not change any of the existing fields.
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
Bug Fixes
get_metafieldsmacro to support multiple reference values, ensuring compatibility with both the Shopify GraphQL API (ex: 'PRODUCTVARIANT' from theproduct_variantsource) and the deprecated REST API (previously 'variant' forproduct_variant). See the Shopify API docs for more information.shopify__product_variant_metafieldsandshopify__product_image_metafieldsto ensure all metafields fromstg_shopify__metafieldare properly included, preventing loss of existing records.id_column_overrideinget_metafieldsmacros to to handle non-standard primary key column names (variant_idfromstg_shopify__product_variant,product_image_idfromstg_shopify__product_imagewhereimageis the default reference value). The override ensures models explicitly call the correct column names.Under the Hood
shopify_metafield_dataseed to validate the functionality of theget_metafieldsmacro, ensuring it correctly retrieves metafield data for all supported reference values.PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
Before marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please share any and all of your validation steps:
Validated that metafields supported multiple field value types, using integration tests seed data on BigQuery, Postgres, and Snowflake.
Validation tests also passed.
If you had to summarize this PR in an emoji, which would it be?
🪛