Skip to content

Deprecated usage of type field name #993

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

Merged
merged 1 commit into from
Oct 11, 2021

Conversation

sliverc
Copy link
Member

@sliverc sliverc commented Oct 10, 2021

Deprecating of #376

Description of the Change

Partial support for field name type was added in #376 but only for non polymorphic fields.

However as per specification type must not be a field name and therefore must be forbidden in DJA as well.

Some dependents might depend on being allowed to have a field name type so deprecating it now and remove it in next major version.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

This support was added in django-json-api#376 but only for non polymorphic fields.

However as per specification [0] `type` must not be a field name and
therefore must be forbidden in DJA as well.

Some dependents might depend on being allowed to have a field
name `type` so deprecating it now and remove it in next major version.

[0] https://jsonapi.org/format/#document-resource-object-fields
@sliverc sliverc requested a review from n2ygk October 10, 2021 18:30
@sliverc sliverc force-pushed the invalid_type_field branch from 47128f3 to a79a0a0 Compare October 10, 2021 18:31
@codecov
Copy link

codecov bot commented Oct 10, 2021

Codecov Report

Merging #993 (a79a0a0) into master (e17ea57) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #993   +/-   ##
=======================================
  Coverage   96.84%   96.84%           
=======================================
  Files          65       65           
  Lines        3929     3933    +4     
=======================================
+ Hits         3805     3809    +4     
  Misses        124      124           
Impacted Files Coverage Δ
rest_framework_json_api/parsers.py 98.59% <ø> (ø)
example/tests/test_model_viewsets.py 100.00% <100.00%> (ø)
tests/test_serializers.py 100.00% <100.00%> (ø)
example/serializers.py 89.78% <0.00%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e17ea57...a79a0a0. Read the comment docs.

@auvipy
Copy link
Contributor

auvipy commented Oct 11, 2021

won;t this break that PR? can any alternative name could be used?

@sliverc
Copy link
Member Author

sliverc commented Oct 11, 2021

won;t this break that PR? can any alternative name could be used?

The issue is that type is per JSON:API specification a reserved word for the resource type and therefore it may not be used as field name. So PR #376 should not have been merged in the first place which added a workaround to be able to use type as field name in some circumstances.

@auvipy
Copy link
Contributor

auvipy commented Oct 11, 2021

ok. fair enough. can field_type be used instead? don't get annoyed by my questions btw

@n2ygk n2ygk merged commit 06c3ef4 into django-json-api:master Oct 11, 2021
@sliverc sliverc deleted the invalid_type_field branch October 11, 2021 16:57
@sliverc
Copy link
Member Author

sliverc commented Oct 11, 2021

@auvipy If you are talking about if an application using DJA already has a serializer with a field name type. Yes in such a case you can simply rename the field name to field_type and it will work.

@auvipy
Copy link
Contributor

auvipy commented Oct 11, 2021

thats great!

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.

3 participants