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

Reducing query counts on processing and removing 1+N queries in Admin View #118

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

Conversation

chadgates
Copy link
Contributor

message, _ = Message.objects.create_from_as2message(
as2message=as2message,
payload=payload,
filename=original_filename,
direction="OUT",
status="P",
)

# Check if we're inside an atomic block, if not, commit immediately to store the message
if not transaction.get_connection().in_atomic_block:
Copy link
Owner

Choose a reason for hiding this comment

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

what is the reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had this in my branch and was a left over from here #45. Issue is, the message is not stored before sending, and if partner sends an MDN back before the message is commited to DB, then the MDN fails and triggers a "no message found" error. This should prevent this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for this is, because tests fun in atomic transaction and therefore this will fail. The behavior of the django test is different than when running the command "normally". transaction.commit() would fail if running inside an atomic transaction.

Copy link
Owner

Choose a reason for hiding this comment

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

We should not be writing code to handle for tests, this needs to be handled differently. Can be picked later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't know how anyone would be using this command - someone might wrap this command in an atomic transaction which would then fail this too probably. So I don't see this as "for tests"...
Originally I was going to use a config flag if we are in test mode or not, but then we even have more "only test specific" code.

Let me know if you want me to simple remove this from this PR, then I will and you can decide at a later point how you want this.

Copy link
Owner

Choose a reason for hiding this comment

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

lets move it out of the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if not transaction.get_connection().in_atomic_block:
transaction.commit()

message.organization = org
Copy link
Owner

Choose a reason for hiding this comment

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

add a comment on why you are doing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will add.

@@ -460,6 +463,13 @@ def status_icon(self):

def send_message(self, header, payload):
"""Send the message to the partner"""

if self.organization_id and self.partner_id:
self.organization = self.organization or Organization.objects.get(
Copy link
Owner

Choose a reason for hiding this comment

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

reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have noted that under certain circumstances (i.e. update_or_create) will not keep the related object inside the model. So, in case that this is happening, then we pull it once more, but we are not pulling it, when it exists already, thus not executing another query.

query for query in queries if "SAVEPOINT" not in query["sql"]
]

# number of queries should be 9
Copy link
Owner

Choose a reason for hiding this comment

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

9 or 13?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will change.

@abhishek-ram
Copy link
Owner

abhishek-ram commented Sep 27, 2024

pyas2/management/commands/sendas2message.py:7:1 [W] W0611 Unused transaction imported from django.db [pylint]

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.

2 participants