Feature: Add setting DEFAULT_PK_FIELD_NAME #446

noamsto · 2023-12-25T13:16:33Z

This PR was inspired by #159.
I took a different approach which is less granular but easier to implement.
If a fine-grain control is needed it can be implemented in the future.

Description

Add a DEFAULT_PK_FIELD_NAME setting which default to "pk".
This should affect all builtin Django CRUDs, and is backward compatible.

Types of Changes

Issues Fixed or Closed by This PR

Closes Possibility to rename pk to id or something else #121

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Signed-off-by: Noam Stolero <nstolero@barracuda.com>

noamsto · 2023-12-25T13:19:53Z

@bellini666
What do you think of this?
I know #159 is abandoned and this is a partial solution, but I know I will use it at least :).

I can add more tests if you think are needed.

Also, need your help regarding this: https://github.com/strawberry-graphql/strawberry-graphql-django/pull/446/files#diff-87c07c1c9f6db39e2dacd257d14f3123d74b65d0a1db4029364d69f4a707013cR55

bellini666

@noamsto I think this is mostly fine, and I commented on the issue you requested my attention, although I'm not sure it is the correct option in there

strawberry_django/filters.py

strawberry_django/mutations/fields.py

bellini666 · 2023-12-26T22:26:59Z

strawberry_django/mutations/mutations.py

@@ -28,6 +30,7 @@
 )

 _T = TypeVar("_T")
+settings = strawberry_django_settings()


I was thinking,
Maybe it's redundant here?
Anyway key_attr is propagating to DjangoMutationCUD and there it will be handled for Update and Delete right?

strawberry_django/filters.py

codecov-commenter · 2023-12-26T22:37:33Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f4f77a3) 87.73% compared to head (f1575b6) 87.71%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
strawberry_django/mutations/fields.py	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #446      +/-   ##
==========================================
- Coverage   87.73%   87.71%   -0.02%     
==========================================
  Files          37       37              
  Lines        3130     3143      +13     
==========================================
+ Hits         2746     2757      +11     
- Misses        384      386       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

for more information, see https://pre-commit.ci

Signed-off-by: Noam Stolero <nstolero@barracuda.com>

bellini666

Looking good! :)

I think the only thing missing is is the DjangomodelFilterInput

I was thinking about it and probably what we want to do it to create it dynamically if DEFAULT_PK_FIELD_NAME != "pk" like I suggested at the point where it is being used (it is only used in one place) and save it in a global variable for further usage.

What do you think?

noamsto · 2024-01-07T09:26:15Z

Looking good! :)

I think the only thing missing is is the DjangomodelFilterInput

I was thinking about it and probably what we want to do it to create it dynamically if DEFAULT_PK_FIELD_NAME != "pk" like I suggested at the point where it is being used (it is only used in one place) and save it in a global variable for further usage.

What do you think?

I don't mind doing that, though the usage is in a different file; defined in: strawberry_django/filters.py, used in: strawberry_django/fields/types.py it's a bit confusing no?

It is also used in test_types.py wouldn't it break the test?

noamsto · 2024-01-08T11:31:32Z

@bellini666 ,
I see there are plenty of places which are declaring pk
I feel like there maybe much more work here than I initially thought.

I need to revisit all the occurrences and see what are necessary changes:
Files with 'pk':

If you think there are places which I mentioned that irrelevant please tell.

I added Mutation test and might add others as well as I progress. It might take me a while.

sourcery-ai

Hey @noamsto - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 5 issues found
🟢 Security: all looks good
🟡 Testing: 5 issues found
🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

strawberry_django/filters.py

strawberry_django/mutations/fields.py

tests/filters/test_filters.py

tests/test_queries.py

…ions

noamsto added 2 commits December 25, 2023 15:07

Add DEFAULT_PK_FIELD_NAME setting

e8a1d4f

Signed-off-by: Noam Stolero <nstolero@barracuda.com>

Make CUD use DEFAULT_PK_FIELD_NAME

e4dc86d

Signed-off-by: Noam Stolero <nstolero@barracuda.com>

noamsto changed the title ~~Query set key attr~~ Feature: Add setting DEFAULT_PK_FIELD_NAME Dec 25, 2023

bellini666 reviewed Dec 26, 2023

View reviewed changes

noamsto and others added 4 commits December 31, 2023 15:24

Add unit-test for custom pk

b3ab5a2

[pre-commit.ci] auto fixes from pre-commit.com hooks

68d4f3e

for more information, see https://pre-commit.ci

Fix unit-tests

eb58113

Signed-off-by: Noam Stolero <nstolero@barracuda.com>

'key_attr' default value is handled in DjangoMutationCUD

f1575b6

Signed-off-by: Noam Stolero <nstolero@barracuda.com>

noamsto force-pushed the query-set-key-attr branch from 337f1ee to f1575b6 Compare December 31, 2023 15:14

noamsto requested a review from bellini666 January 4, 2024 13:01

bellini666 reviewed Jan 6, 2024

View reviewed changes

patrick91 added 7 commits May 22, 2024 23:57

Merge branch 'main' into query-set-key-attr

3a85d4b

Dynamically create input filter

0f35167

Create the input type only when needed

a06e98f

Ignore type error

4f7aaca

Add test

fd835a0

Ignore another typing error

6c4fede

Fix linter

490c897

patrick91 marked this pull request as ready for review May 22, 2024 22:42

patrick91 requested a review from bellini666 May 22, 2024 22:42

sourcery-ai bot reviewed May 22, 2024

View reviewed changes

refactor: Use DEFAULT_PK_FIELD_NAME when parsing data for CUD mutat…

4cb46ed

…ions

bellini666 approved these changes May 26, 2024

View reviewed changes

bellini666 merged commit 971231b into strawberry-graphql:main May 26, 2024
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add setting DEFAULT_PK_FIELD_NAME #446

Feature: Add setting DEFAULT_PK_FIELD_NAME #446

noamsto commented Dec 25, 2023 •

edited

Loading

noamsto commented Dec 25, 2023

bellini666 left a comment

bellini666 Dec 26, 2023

noamsto Dec 27, 2023

codecov-commenter commented Dec 26, 2023 •

edited

Loading

bellini666 left a comment

noamsto commented Jan 7, 2024

noamsto commented Jan 8, 2024 •

edited by bellini666

Loading

sourcery-ai bot left a comment

Feature: Add setting DEFAULT_PK_FIELD_NAME #446

Feature: Add setting DEFAULT_PK_FIELD_NAME #446

Conversation

noamsto commented Dec 25, 2023 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

noamsto commented Dec 25, 2023

bellini666 left a comment

Choose a reason for hiding this comment

bellini666 Dec 26, 2023

Choose a reason for hiding this comment

noamsto Dec 27, 2023

Choose a reason for hiding this comment

codecov-commenter commented Dec 26, 2023 • edited Loading

Codecov Report

bellini666 left a comment

Choose a reason for hiding this comment

noamsto commented Jan 7, 2024

noamsto commented Jan 8, 2024 • edited by bellini666 Loading

sourcery-ai bot left a comment

Choose a reason for hiding this comment

noamsto commented Dec 25, 2023 •

edited

Loading

codecov-commenter commented Dec 26, 2023 •

edited

Loading

noamsto commented Jan 8, 2024 •

edited by bellini666

Loading