-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🐛 Source Hubspot: Add missing types for the Workflows
stream schema
#39314
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…sing-schema-types
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.
Just comments outside of the scope of this fix
@@ -158,6 +158,7 @@ | |||
"properties": { | |||
"sourceApplication": { | |||
"description": "Application details of the workflow update source", | |||
"type": ["null", "object"], |
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.
Out of curiosity, what is the default type if this is not provided? Should this have been caught by CATs?
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.
This is a great question.
The data structure remains the same, but the schema was missing these types only.
I wonder why the CAT didn't catch such things,
I assume we don't have the type
field presence check yet for the schemas, as a separate testing scenario for the CAT, thus we can easily bypass such things.
Correct me if I'm wrong here.
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 definitely expect this test to fail in that case. Maybe we haven't configured the validator properly..?
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.
The type
field is optional for the JsonSchema, in general, so probably there should be an additional check on top of the existing validator to validate its presence.
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.
What is the default value? I could not find this information. If it is the case, we either need to have destinations support this or update our CATs. The latter seems to build more leverage and seems easier to implement too. Can we at least create an issue for that?
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 think, in this case; it depends on the destination
used. There is no default
value for the source; in this case, it's a matter of how the destination treats the missing
types. For instance, BigQuery
raises an Exception that there is no type, but it expected any of Null, Object
based on the actual data type faced.
What
Resolving:
How
type: ["null", "object"]
where the schema declares it for the Workflows streamUser Impact
No impact is expected, not a breaking change.
Can this PR be safely reverted and rolled back?