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

[fix] Avoid redundant SQL write query during CA/Cert creation generation #168

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

Conversation

SitaGanesh
Copy link

@SitaGanesh SitaGanesh commented Dec 12, 2024

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Reference to Existing Issue

Closes #120.

Description of Changes

Modified a test to the test_new function in test_cert.py to measure the number of database queries before and after the optimization. Ensuring that there should be at least 1 less query.

Screenshot

code

@SitaGanesh
Copy link
Author

Sir @nemesifier, I would like to contribute to this organization, I believe I have made changes correctly as you have mentioned. I want to move forward in the next challenges that you assign me, I will do my best to tackle them by facing them. Could you guide me on what to do further to prove myself?

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Thank you for your patience @SitaGanesh!

We've been busy completing some projects before Christmas and the End of the year and couldn't look into this yet.

Please see my comments below.

Please also ensure tests and QA checks pass locally, read the contributing guidelines for more info on QA checks and tests:
https://openwisp.io/docs/dev/developer/contributing.html

django_x509/tests/test_cert.py Show resolved Hide resolved
django_x509/base/models.py Outdated Show resolved Hide resolved
@nemesifier nemesifier changed the title Add an assertion in the tests hat checks the number of queries, comparing beforing and after this patch. [fix] Avoid redundant SQL write query during CA/Cert creation generation Dec 23, 2024
@nemesifier nemesifier added the bug label Dec 23, 2024
@SitaGanesh
Copy link
Author

Thank you @nemesifier for the feedback, and I was awaiting your response

I will review and try to modify the test to utilize assertNumQueries. I apologize for using my own method to implement the code, and I will ensure that it minimizes unnecessary alterations.

Let me repeat what you said for clarification:

  • Update the test case to add the assertNumQueries assertion and manually verify that the number of queries has decreased.

  • The generate variable and the related logic in my code may no longer be necessary, so I need to remove them completely.

Thank you for pointing me to the contributing guidelines and the references for assertNumQueries. I’ll review them carefully, and I need some time, sir, to accomplish this task.
Please confirm if I understood the points correctly.

@nemesifier
Copy link
Member

nemesifier commented Dec 23, 2024

I believe your understanding is correct, take your time. Ping us in the dev chat when ready. Thanks.

@SitaGanesh
Copy link
Author

Hi sir @nemesifier, this is the PR #170 which solves the both cases. I hope I have created a successful PR with all the requirements ment for consideration. If there will be any modification in the code please let me know. I will alter that them.

Co-authored-by: Federico Capoano <[email protected]>
Copy link
Author

@SitaGanesh SitaGanesh left a comment

Choose a reason for hiding this comment

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

I have made the suggested changes by removing the generate variable and simplifying the save method as follows:

def save(self, *args, **kwargs): if not self.pk and not self.certificate and not self.private_key: if not self.serial_number: self.serial_number = self._generate_serial_number() self._generate() super().save(*args, **kwargs)
the test cases goes here:-
test-cases-for-save-method-no-errors-regarding

I have made the suggested changes by using assertNumQueries as follows:

def test_new(self): with self.assertNumQueries(3): # 3 queries to be made cert = self._create_cert() self.assertNotEqual(cert.certificate, '') self.assertNotEqual(cert.private_key, '') x509 = cert.x509 self.assertEqual(x509.get_serial_number(), cert.serial_number) subject = x509.get_subject()

The test cases goes here:-
assertNumQueries-test-case-solved

django_x509/base/models.py Outdated Show resolved Hide resolved
django_x509/tests/test_cert.py Show resolved Hide resolved
SitaGanesh added a commit to SitaGanesh/django-x509 that referenced this pull request Jan 1, 2025
SitaGanesh added a commit to SitaGanesh/django-x509 that referenced this pull request Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] CA/Cert creation generates 2 SQL write queries instead of 1
2 participants