-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix: make nullable django fields also Noneable in event data #394
Conversation
99ba749
to
ace9540
Compare
@mariajgrimaldi Please take a look when you have a chance! |
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 looks good. Let's hope no other events raise the same errors. I'll make a mental note to look for these edge cases when reviewing. Thank you so much!
Is this enough to fix the issue described above?
Thanks for the prompt review! I do want to get this right so now that I think about it, I want to perform additional testing. I was just eager to get this out for review in case of delay, but that was not a concern. Maybe you can also help answer a question about versioning: Since this is one of the first instances where we're "evolving" an existing event without creating a v2 version of it, do you forsee any possible issues of doing that? Technical issues maybe? This change is theoretically backwards-compatible (only making existing fields more permissive), and I also doubt anybody in the open source community has adopted this event given it's limited use for enterprise purposes and age. |
@pwnage101: That's a good question. As you said, I see these changes as backward compatible too. If these events were adopted elsewhere, defaults must've been handled in some way for them to work, so these changes won't affect current implementations. In this case, documentation will be enough to let adopters know. However, if there was already a default for the attributes, we'll have to discuss the implications of the default change further. |
I just finished testing locally, and indeed it works the way I expected. First here's a reproduction of the error before any changes (
After changes to make fields allow None:
|
I'm happy with this PR and feel it's ready to merge if you feel the same. |
ff45dd6
to
11df135
Compare
@mariajgrimaldi please merge at your convenience |
@mariajgrimaldi could you take a look? |
CHANGELOG.rst
Outdated
@@ -18,6 +18,14 @@ __________ | |||
|
|||
|
|||
|
|||
[9.14.1] - 2024-09-12 |
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 you please update the date?
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'll be merging this today. Thank you!
This should hopefully fix the error we are currently seeing when attempting to serialize real-life LC fulfillments outside of unit test environments. The error is exactly described in this ticket for a different event experiencing the same issue: edx/edx-arch-experiments#788 ENT-9213
11df135
to
bbaf754
Compare
This should hopefully fix the error we are currently seeing when attempting to serialize real-life LC fulfillments outside of unit test environments. The error is exactly described in this ticket for a different event experiencing the same issue:
edx/edx-arch-experiments#788
Basically, certain fields are passed into the serializer as None because the actual value in the database is NULL, but the serializer field default is not explicitly set to None which triggers a code path that doesn't tolerate being passed None:
openedx-events/openedx_events/event_bus/avro/serializer.py
Lines 34 to 44 in 3170416
ENT-9213