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

save extended properties only when asked for #316

Merged
merged 9 commits into from Jun 12, 2022

Conversation

AmitPhulera
Copy link
Contributor

Fixes #315

Currently django-celery-results stores all the info about the task to DB whereas the default behavior as per celery should only be storing some basic properties. Extended properties should be saved only when they are requested for. It can be done by setting extended-result flag in the celery config.

The PR checks for that flag while storing the results and proceeds accordingly.

I would be adding some tests for it once we are okay with the approach.

@auvipy auvipy self-requested a review May 25, 2022 09:04
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will need unit tests to cover

@AmitPhulera
Copy link
Contributor Author

@auvipy Thanks for the quick turnaround. Yeah, I am planning to add unit tests for it. Just wanted to be sure that this is the right way to go forward.

@AmitPhulera
Copy link
Contributor Author

Hey @auvipy,
I have added a test to verify the flag works 7edbd42 and also updated previous tests to use the flag 6974a76

@AmitPhulera
Copy link
Contributor Author

Hey @auvipy,
Could you please let me know what would be the next steps in this PR?

@auvipy auvipy requested a review from xirdneh May 30, 2022 10:55
@auvipy auvipy self-requested a review June 2, 2022 08:39
@auvipy
Copy link
Member

auvipy commented Jun 2, 2022

@AllexVeldman can you check this please

django_celery_results/backends/database.py Outdated Show resolved Hide resolved
django_celery_results/backends/database.py Outdated Show resolved Hide resolved
@AmitPhulera
Copy link
Contributor Author

Thanks for your review @AllexVeldman, I have addressed the feedback that you have shared. Let me know if there is anything else that you want me to update.

@lgtm-com
Copy link

lgtm-com bot commented Jun 3, 2022

This pull request introduces 1 alert when merging bde3eef into e174c99 - view on LGTM.com

new alerts:

  • 1 for Signature mismatch in overriding method

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we document the changes?

@AmitPhulera
Copy link
Contributor Author

@auvipy Can you please guide me on what should be the correct place to document it?
The celery docs already have information about the same.
Do you think an entry in changelog would suffice?

@auvipy
Copy link
Member

auvipy commented Jun 11, 2022

If you can please dig the docs and find out any suitable place to add, otherwise an entry to changelog would be alright. but I would request to check the docs more initially.

@AmitPhulera
Copy link
Contributor Author

@auvipy I was actually looking into the docs earlier to figure out if this can be communicated in a better way. Still, the only docs that I came across were these which mention the installation(which points to celery docs), API, and changelog.

I think only entry in changelog makes sense here

If you have any other place in mind, I would be happy to make the changes there.

@auvipy
Copy link
Member

auvipy commented Jun 11, 2022

ok put in change log please

@AmitPhulera
Copy link
Contributor Author

@auvipy I have added the changes in Changelog. See af09b87

I have put it in the minor version bump rather than updating the patch version. Feel free to update as you seem fit.

@auvipy auvipy merged commit ad508fe into celery:master Jun 12, 2022
@AmitPhulera AmitPhulera deleted the respect-extended-result-flag branch June 12, 2022 10:31
@AmitPhulera
Copy link
Contributor Author

Hey @auvipy,
Would you be able to provide an estimate on when a release tag would be created for the change?

@auvipy
Copy link
Member

auvipy commented Jun 14, 2022

this week or week end

@auvipy
Copy link
Member

auvipy commented Jul 7, 2022

this creates a regression, unless their is a fix, going to revert this

@AmitPhulera
Copy link
Contributor Author

@auvipy Can you please elaborate the issue?

@auvipy
Copy link
Member

auvipy commented Jul 7, 2022

new update #326 (comment)

@AmitPhulera
Copy link
Contributor Author

I guess the change is okay, right?

@auvipy
Copy link
Member

auvipy commented Jul 7, 2022

seems yeah, need some doc direction most probably

@valberg
Copy link
Contributor

valberg commented Jul 7, 2022

Could we add a FAQ or something to the docs to inform of this? Maybe even copy the "Getting started" from https://docs.celeryq.dev/en/latest/django/first-steps-with-django.html#django-celery-results-using-the-django-orm-cache-as-a-result-backend and include a note in that?

@auvipy
Copy link
Member

auvipy commented Jul 7, 2022

please go ahead

@valberg
Copy link
Contributor

valberg commented Jul 7, 2022

Here you go :) #328

@G-Rath
Copy link

G-Rath commented Jul 9, 2022

Looks like this resolves #142 aka GHSA-fvx8-v524-8579 🎉

@auvipy
Copy link
Member

auvipy commented Jul 10, 2022

wow

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.

django-celery-results does not respect result_extended flag
5 participants