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

Don't use English for string comparisons #14910

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

dmzoneill
Copy link
Member

SUMMARY

This work is to remove string validation using comparisons of English literals for comparison.
This replaces validation with error/op codes as a universal approach to validation and comparison

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API
AWX VERSION
all

@dmzoneill dmzoneill force-pushed the AAP-20885-20627-english-string-comparisons branch 4 times, most recently from 82f1e2c to a516325 Compare February 21, 2024 18:49
@dmzoneill dmzoneill marked this pull request as draft February 22, 2024 13:06
awx/main/tasks/system.py Outdated Show resolved Hide resolved
@dmzoneill dmzoneill force-pushed the AAP-20885-20627-english-string-comparisons branch 5 times, most recently from 028364d to 1b580d3 Compare February 22, 2024 15:56
@dmzoneill dmzoneill force-pushed the AAP-20885-20627-english-string-comparisons branch from 1b580d3 to e0b9d39 Compare February 28, 2024 16:59
# xxx.rowcount on update/delete shoud be used to determined affected rows.
# Not sure how an expcetion is thrown from update_computed_fields()
else:
logger.debug('Exiting duplicate update_inventory_computed_fields task. {}'.format(str(e)))
Copy link
Contributor

Choose a reason for hiding this comment

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

You've lost the raise in this exception handler.

Copy link
Member Author

Choose a reason for hiding this comment

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

added raise on 805

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, the "did not affect any rows" message is coming from Django instead of the db in this case: https://github.com/django/django/blob/eff21d8e7a1cb297aedf1c702668b590a1b618f3/django/db/models/base.py#L1107

It's that one in particular, since i.update_computed_fields() makes use of .save(update_fields=...).

You are correct that in this case the exception won't get a sqlstate, so this flow of logic looks ok to me.

awx/conf/settings.py Outdated Show resolved Hide resolved
@dmzoneill dmzoneill force-pushed the AAP-20885-20627-english-string-comparisons branch from e0b9d39 to 901aad7 Compare March 4, 2024 16:16
@dmzoneill dmzoneill marked this pull request as ready for review March 4, 2024 17:13
@dmzoneill dmzoneill force-pushed the AAP-20885-20627-english-string-comparisons branch from 901aad7 to fc63655 Compare March 4, 2024 17:13
if cause and hasattr(cause, 'sqlstate'):
sqlstate = cause.sqlstate
sqlstate_str = psycopg.errors.lookup(sqlstate)
logger.error('SQL Error state: {} - {}'.format(sqlstate, sqlstate_str))
raise
Copy link
Contributor

Choose a reason for hiding this comment

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

While I would expect that this exception block would always get a sqlstate, if we are coding defensively the raise should be outside of the if block, just in case that isn't so.

Copy link
Contributor

@jbradberry jbradberry left a comment

Choose a reason for hiding this comment

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

This looks good, my only recommendation now is to probably code more defensively around the raise in utils/common.py.

@dmzoneill dmzoneill force-pushed the AAP-20885-20627-english-string-comparisons branch 6 times, most recently from 8733bb1 to 7000b6a Compare March 6, 2024 15:14
@dmzoneill dmzoneill force-pushed the AAP-20885-20627-english-string-comparisons branch 4 times, most recently from c0b2ddd to 6ee96d8 Compare March 6, 2024 16:34
@dmzoneill dmzoneill marked this pull request as draft March 7, 2024 00:18
@dmzoneill dmzoneill force-pushed the AAP-20885-20627-english-string-comparisons branch from 6ee96d8 to 2fff717 Compare March 7, 2024 10:17
@gundalow gundalow changed the title AAP 20885 20627 English string comparisons Don't use English for string comparisons Mar 7, 2024
@dmzoneill dmzoneill force-pushed the AAP-20885-20627-english-string-comparisons branch 3 times, most recently from f019fc4 to a1e9a71 Compare March 7, 2024 14:30
@jbradberry jbradberry self-requested a review March 7, 2024 16:32
@@ -786,11 +795,17 @@ def update_inventory_computed_fields(inventory_id):
return
i = i[0]
try:
i.update_computed_fields()
if i.update_computed_fields() == 0:
logger.debug('Duplicate update_inventory_computed_fields task.')
Copy link
Contributor

@jbradberry jbradberry Mar 7, 2024

Choose a reason for hiding this comment

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

Per offline conversation, I do not believe that this logging will fire off.

Copy link
Contributor

@jbradberry jbradberry left a comment

Choose a reason for hiding this comment

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

New comment.

logger.debug("Finished updating inventory computed fields, pk={0}, in {1:.3f} seconds".format(self.pk, time.time() - start_time))
return rowcount
Copy link
Contributor

@jbradberry jbradberry Mar 7, 2024

Choose a reason for hiding this comment

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

I oppose this change for the following reasons:

  • making changes here has crept out of the scope of this PR
  • the bulk_update is not the only save occurring in this method, so returning only its row count isn't very idiomatic
  • this does absolutely nothing to address the fact that iobj.save(update_fields=computed_fields.keys()) is going to raise the DatabaseError("Save with update_fields did not affect any rows.") exception in the case we care about

@dmzoneill dmzoneill force-pushed the AAP-20885-20627-english-string-comparisons branch 2 times, most recently from eeda300 to fbeb0de Compare March 8, 2024 14:35
jbradberry
jbradberry previously approved these changes Mar 8, 2024
@dmzoneill dmzoneill marked this pull request as ready for review March 11, 2024 14:12
@dmzoneill dmzoneill force-pushed the AAP-20885-20627-english-string-comparisons branch from fbeb0de to 7b8278e Compare March 11, 2024 14:19
@dmzoneill dmzoneill enabled auto-merge (rebase) March 11, 2024 17:01
@dmzoneill dmzoneill merged commit ca8085f into devel Mar 11, 2024
21 checks passed
@dmzoneill dmzoneill deleted the AAP-20885-20627-english-string-comparisons branch March 11, 2024 20:07
@Klaas-
Copy link
Contributor

Klaas- commented Mar 21, 2024

possible regression because of this change #15016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants