Skip to content
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

bugfix/keyerror-at-fields-to-jsonapi-schema-rendering #46

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alidaw
Copy link

@alidaw alidaw commented Feb 29, 2024

The implementations out for review are used to fix KeyError happening at fields to JSON:API schema rendering due to failing logic applied due iteration. See related issue @ #45.

@alidaw alidaw force-pushed the bugfix/keyerror-at-fields-to-jsonschema-rendering branch 2 times, most recently from 32d4217 to 89ce44c Compare February 29, 2024 20:16
@vladmunteanu
Copy link
Owner

@alidaw Thanks for submitting the issue and the PR! I will take a closer look in the next couple of days.
In the meantime, any chance you could add a test that fails when this change is not applied?

@vladmunteanu
Copy link
Owner

The openapi support is really experimental, and I would've preferred the implementation to be done with apispec.
Sadly, that library doesn't play well with the serialization hooks implemented in marshmallow-jsonapi which is why I started playing around with a new implementation of marshmallow-jsonapi here: https://github.com/vladmunteanu/marshmallow-jsonapi-exp

Didn't have time to finish it, but I may get a chance to do that soon. If you're passionate about this subject, feel free to have a look at the marshmallow-jsonapi-exp idea. I think a much cleaner solution would be achievable by using apispec instead of the custom logic added in starlette-jsonapi.

As the field called id according to rules guided
by JSON:API is required it's processing must be
be excluded in iteration's control structure
used to apply requirment state to fields being
either child of property called attributes or
relationships.
@alidaw alidaw force-pushed the bugfix/keyerror-at-fields-to-jsonschema-rendering branch from 89ce44c to a8cba9c Compare March 1, 2024 05:17
@alidaw
Copy link
Author

alidaw commented Mar 1, 2024

Thx in advance! I needed to overwrite the commit I applied to fix the issue via interactive rebase. Checking if key called attributes exists in dictionary before accessing it to set requirement state of field addressed in iteration wouldn't solve the issue as the issue was caused by not ignoring the field called id there.

As the field called id according to rules guided by JSON:API is required, it's processing must be be excluded somewhere on top of your iteration's control structure used to apply requirment state to fields being either child of property called attributes or relationships.
Otherwise, if field called id won't be excluded there and would be the first field processed in iteration where key called attributes haven't been set yet, we would run into KeyError. KeyError at key called relationships anyway won't be an issue there as you already take care to apply it only for fields of kind equaling relationship, which according to JSON:API shouldn't be the case for field called id.

@alidaw
Copy link
Author

alidaw commented Mar 1, 2024

In addition to the commit bearing the bug fix I've added a commit used to expand the unit tests of tests.test_openapi module with three tests:

  • test_fields2jsonschema_conversion_if_id_first_item_iterated
  • test_fields2jsonschema_conversion_if_id_last_item_iterated
  • test_fields2jsonschema_conversion_if_id_in_between_item_iterated

The three tests run green. If I comment out the bug fix I've applied, then the tests fail with the same reason (KeyError('attributes')) as the fetching of API docs generated via starlette-jsonapi fails where call of starlette-jsonapi.openapi.JSONAPISchemaConverter.fields2jsonschema() method is involved. So the tests I've implemented prove that the bug fix I've provided would fix the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants