-
Notifications
You must be signed in to change notification settings - Fork 17
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-stripe-native: make created optional for Accounts #1978
source-stripe-native: make created optional for Accounts #1978
Conversation
7b59ef4
to
122b031
Compare
@@ -151,6 +151,8 @@ class Accounts(BaseStripeObjectWithEvents): | |||
"account.updated": "u", | |||
} | |||
|
|||
# Accounts docs returned in account.updated events may not have a created field. | |||
created: int | None = None |
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.
Is there a way to get this to schematize as "not required to be present, but if it is present, it must be int
"? I think that's what we want: The field is either present as an int, or not present at all. It shouldn't ever be present as an actual null
value.
The schema coming from this is saying it can be a null
, and also giving it a default null
value (we don't want that default, for sure). Ideally the schema would be this:
created: {
"type": "integer",
"title": "created"
}
...and it would not be present in the list of required
fields.
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 looked through a handful of Pydantic discussions, and I haven't found any straightforward way to have an optional, non-nullable field. I've looked in Pydantic's docs, and it looks like optional fields always have to have a default value.
To avoid saying created
can be null
in the schema, we could have a default value like -1
, then overwrite created
with the event's timestamp whenever we see that it's -1
. That's not 100% ideal, but I'm not seeing a better way to handle this right now. What do you think about that approach?
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 have used this hack for not schematizing the default value before - does that help here?
I think we need a way to represent this correctly. If it the field is either present and is some integer, or is not present at all, we need to have the schema say that. A default of null
is not ideal, but a default of -1
is even worse since that is not what the value of the field is, and since it's a unix timestamp might be problematic in other ways.
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.
That code you linked did help - thank you! I'll get the PR updated.
Sometimes, `account.updated` events do not have `created` in the document, causing failures when we validate the document against its model. Allowing a default value of `None` optional will avoid these failures and reflect the API's actual response. Note: We remove the `None` default value with a lambda to keep the schema more inline with what actually happens: the `created` field is either present as an `int` or it's not present at all.
122b031
to
34d27ef
Compare
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.
LGTM
Description:
Sometimes,
account.updated
events do not have acreated
field in the document, causing failures when we validate the document against its model. Makingcreated
optional will avoid these failures and reflect the API's actual response. The default value ofNone
is removed from the schema to keep it aligned with how we understand thecreated
field: it's either present as anint
or it's not present at all. In other words, it's an optional, non-nullable field.Note: I've only noted that this occurs for
account.updated
events for when the user's own Stripe account is created. I couldn't confirm if allaccount.updated
events havecreated
missing in the document, or ifAccounts
documents really don't havecreated
fields at all. If this becomes an issue later, we will need to re-evaluate how we backfillAccounts
since we rely on acreated
field being present.Discover snapshot change is expected due to switching
created
from a required field to an optional field forAccounts
.Workflow steps:
(How does one use this feature, and how has it changed)
Documentation links affected:
(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)
Notes for reviewers:
Tested on a local stack. Confirmed
Accounts
does not cause shard failures whenaccount.updated
events do not have acreated
field in the document.Existing tasks will need to be re-discovered since we're changing
created
from required to optional forAccounts
.This change is