fix: ensure that "empty" object fields on advisories are handled correctly #56
Conversation
| transformed to make it easier to work with | ||
| """ | ||
|
|
||
| field_project: EntityReferenceField | None |
There was a problem hiding this comment.
note: I've included this field as its technically possible, even though I think its required on the upstreams end
|
|
||
|
|
||
| class Advisory(Node): | ||
| class AdvisoryBase(Node): |
There was a problem hiding this comment.
note: I've had to split this into three types because Python doesn't allow you to change the type of a field when inheriting, regardless of the variance of the type
i.e. ideally we'd do something like
class AdvisoryRaw(Node):
field_sa_description: RichTextField | list[None]
class Advisory(AdvisoryRaw):
field_sa_description: RichTextField
but that is not supported
| # https://youtrack.jetbrains.com/issue/PY-58714/False-positive-TypedDict-has-missing-key-when-using-unpacking | ||
| sa_advisory: drupal.Advisory = { | ||
| **raw_advisory, | ||
| 'field_project': None, |
There was a problem hiding this comment.
note: since this represents a relationship with another entity whose ID we will look sometimes, I didn't think it was a good idea to try and have a fallback for this
for the other fields though I felt that was safe and worth doing as it lets us avoid changing most other code
|
Converting to draft for now because having through about it a bit further I'm thinking it might be better for now to just handle this in the specific place the fields are being used since that'll actually only be one place currently (when we build the credits) as the project field should never ever be missing 🤔 (it is important that the types reflect the actual structure of the JSON though so either way we def want that part of the change) |
G-Rath
force-pushed
the
normalize
branch
3 times, most recently
from
cece987 to
71f9ca8
Compare
So it turns out in some advisories some instances of this field are actually empty arrays which I think is because PHP uses associative arrays for what are "objects" in JavaScript, `dict`s in Python, and hashes in Ruby. This means an empty associative array is the same as an empty indexed array, so when being turned into JSON PHP cannot know if it should be an actual array or an empty object so it goes with the former. While that sounds reasonable, what I'm unsure of is why Drupal would be returning an empty array rather than `null` when there isn't a value for an optional relationship - so this might actually be a bug on the upstreams end, but either way our types should represent the JSON we have today. The check we currently have for this field does guard against this, but technically does not narrow the type in a way that `mypy` can understand so I've changed it to use `isinstance`. (this is a smaller version of #56 which is focused on the impacted field that we're currently using)
3 participants
So it turns out in some advisories some of these fields are actually empty arrays which I think is because PHP uses associative arrays for what are "objects" in JavaScript,
dicts in Python, and hashes in Ruby. This means an empty associative array is the same as an empty indexed array, so when being turned into JSON PHP cannot know if it should be an actual array or an empty object so it goes with the former.While that sounds reasonable, what I'm unsure of is why Drupal would be returning an empty array rather than
nullwhen there isn't a value for an optional relationship - so this might actually be a bug on the upstreams end, but ultimately we need to handle it here so I've introduced a utility function for loading advisories from JSON which normalizes these properties.Note that right now this isn't actually fixing anything specific because we're not using any of the properties that actually are arrays in some advisories, but we will do in future as part of processing credits.
Also, having bitten the bullet on introducing a
utils.pyalso means we can relocate some of our other utility functions that current live in the main scripts