refactor: reduce DB updates by traits update requests without real changes in the data

[devapro]

Thanks for submitting a PR! Please check the boxes below:

• I have added information to docs/ if required so people know about the feature!
• I have filled in the "Changes" section below?
• I have filled in the "How did you test this code" section below?
• I have used a Conventional Commit title for this Pull Request

Changes

It is common case for mobile app to set traits on the app start, because some properties can be changed offline or by request from another device with the same account etc. However in this case db is update even if no changes in the data. This change should reduce updates by getting list of traits from db than compares the existing and incoming data, and only add/update when the data actually changes. This strategy drastically reduces unnecessary UPDATE operations in case when the clients send requests with the same data.

How did you test this code?

manually

Please describe.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
docs	⬜️ Ignored (Inspect)	Visit Preview		Aug 9, 2024 1:48pm

[vercel]

@devapro is attempting to deploy a commit to the Flagsmith Team on Vercel.

A member of the Team first needs to authorize it.

[sentry-io]

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: api/environments/identities/traits/views.py

Function	Unhandled Issue
bulk_create	KeyError: 'identifier' /api/v1/traits/bulk/ Event Count: 1

---

_{Did you find this useful? React with a 👍 or 👎}

[github-actions]

Uffizzi Ephemeral Environment deployment-56255

☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/4451

📄 View Application Logs etc.

What is Uffizzi? Learn more!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.18%. Comparing base (5cbdd7f) to head (39d82cd).
Report is 121 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4451      +/-   ##
==========================================
+ Coverage   96.85%   97.18%   +0.32%     
==========================================
  Files        1170     1163       -7     
  Lines       38812    40289    +1477     
==========================================
+ Hits        37592    39153    +1561     
+ Misses       1220     1136      -84     

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

[zachaysan]

Looks good. Though it seems like CI has failed for this test, perhaps try re-running the test suite with an empty commit?

[matthewelwell]

Though it seems like CI has failed for this test, perhaps try re-running the test suite with an empty commit?

This is actually an issue in our CI workflows that don't work correctly for external contributors. We are working to resolve this.

I've added a comment to a specific part of the diff but, in general, I'd also like to see a test / some tests written for this logic.

[matthewelwell]

Hmm... this actually looks like it changes the behaviour of the endpoint which could have an effect on clients. Previously, we are always returning the traits that were sent (assuming they were valid). Now we're sending an empty list if none of the traits are actually updated / created.

Perhaps we could do something like the following:

all_traits = [*traits, *SDKCreateUpdateTraitSerializer(instance=existing_traits, many=True).data]
return Response(all_traits)

[devapro]

Thank you for review.
I made changes, now traits always added to the response.
Moved all logic of updating traits to new method, I think it will be a bit easy to read an understand in this case.

[matthewelwell]

Thanks for the changes @devapro . I've added a couple more comments.

As per my previous review though, it's still missing any sort of testing coverage.

[matthewelwell]

@devapro just checking in to see if you had time to look at my recent review comments and whether you might have time to update the PR?

[devapro]

Sorry for long response, will check your comments next week.

[devapro]

Thanks for the changes @devapro . I've added a couple more comments.

As per my previous review though, it's still missing any sort of testing coverage.

I checked existing unit tests and looks like it is covered all logic that I refactored.
I think will be good to add test that checks case when db is not called, but my knowledge in python very limited, I didn't find an example of how to do it in current code.

[matthewelwell]

This is looking better. I've left 2 fairly minor additional comments, but I think it's pretty close.

[matthewelwell]

Suggested change

	def _update_traits(self, request):
	def _update_traits(self, request: Request) -> None:

[devapro]

✔️

[matthewelwell]

I think will be good to add test that checks case when db is not called, but my knowledge in python very limited, I didn't find an example of how to do it in current code.

You can just copy one of the existing tests, manipulate the data to be what you want and then use the django_assert_num_queries fixture to do something like:

with django_assert_num_queries(0):
    ... 

You'll find plenty of usages of this fixture throughout the code base.

[devapro]

I think will be good to add test that checks case when db is not called, but my knowledge in python very limited, I didn't find an example of how to do it in current code.

You can just copy one of the existing tests, manipulate the data to be what you want and then use the django_assert_num_queries fixture to do something like:

with django_assert_num_queries(0):
    ... 

You'll find plenty of usages of this fixture throughout the code base.

I can't find existing test for method that I changed. It is not simple for me to understand how to write new test for specific method.

Didn't manage to run tests locally, for running project I am using docker + mounting specific files with changes, because can't install all required dependencies.

See conversation above.