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

Provide meaningful string representation for DynamoDB Attr/Condition #3259

Open
1 of 2 tasks
rajveerappan opened this issue May 6, 2022 · 9 comments
Open
1 of 2 tasks
Labels
dynamodb feature-request This issue requests a feature. needs-review p3 This is a minor priority issue resources

Comments

@rajveerappan
Copy link

Describe the feature

Currently printing/logging a FilterExpression will show something like:

<boto3.dynamodb.conditions.AttributeExists object at 0x1041021f0>
<boto3.dynamodb.conditions.And object at 0x104102370>
<boto3.dynamodb.conditions.In object at 0x1041027f0>

which is not very helpful. Implementing __str__ on the boto3 Attr and ConditionBase classes will instead result in logging like:

attribute_exists(mykey)
(mykey = foo AND myotherkey = bar)
mykey IN foo

Use Case

Improved human readable logging of DynamoDB FilterExpressions

Proposed Solution

#3254

The way this is implemented is by turning an expression_format which looks like {0} {operator} {1} into something that looks like {values[0]} {operator} {values[1]} which can be passed to the string format method using named indices.

Other Information

An alternative solution would be to change all the expression_formats themselves to use a format like {values[0]} {operator} {values[1]} but that would be a larger change and require updating the end consumers of ConditionBase#get_expression.

Avoiding the regex could also be accomplished by explicitly setting a new str_expression_format field that uses a format like {values[0]} {operator} {values[1]}.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

SDK version used

latest

Environment details (OS name and version, etc.)

all

@rajveerappan rajveerappan added feature-request This issue requests a feature. needs-triage This issue or PR still needs to be triaged. labels May 6, 2022
@tim-finnigan
Copy link
Contributor

Thanks @rajveerappan for the feature request. I’ll plan to bring this up for discussion with the team next week to get their thoughts and will share any updates here.

@tim-finnigan tim-finnigan added dynamodb and removed needs-triage This issue or PR still needs to be triaged. labels May 6, 2022
@aBurmeseDev aBurmeseDev added the p3 This is a minor priority issue label Nov 3, 2022
@tim-finnigan
Copy link
Contributor

Thanks for your patience - this fell off our radar but still needs to be prioritized for team review. It looks like this may actually be a duplicate of an older issue: #1202. We try to consolidate overlapping issues in order to making tracking easier. Please let us know if there are any distinctions you want to highlight between these issues, otherwise I think we should continue tracking this in #1202.

@tim-finnigan tim-finnigan added response-requested Waiting on additional information or feedback. resources labels Nov 18, 2022
@github-actions
Copy link

Greetings! It looks like this issue hasn’t been active in longer than five days. We encourage you to check if this is still an issue in the latest release. In the absence of more information, we will be closing this issue soon. If you find that this is still a problem, please feel free to provide a comment or upvote with a reaction on the initial post to prevent automatic closure. If the issue is already closed, please feel free to open a new one.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Nov 23, 2022
@rajveerappan
Copy link
Author

apologies, this fell off my radar as well. I had a PR open for this that had some feedback. I can try to clean that up.

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional information or feedback. labels Nov 23, 2022
@tibbe
Copy link

tibbe commented Mar 7, 2023

As I mentioned in #3608, this would also be a relatively large improvements to testing using Stubber. Today a test that fails because of mismatched conditions will have an error like (simplified):

Expected:

'KeyCondition': <boto3.dynamodb.conditions.And object at 0x104102370>

Got:

'KeyCondition': <boto3.dynamodb.conditions.And object at 0x1041027f1>

because lack of printing of e.g. the And condition. This makes it the message not much more informative than "the test failed due to some error in the condition".

Once pretty-printing for conditions is fixed we could (in some future PR) also support diffing for test failures using Stubber, like e.g. pytest does in general. Diffing the expected and actual arguments to a boto3 service call now would be not as helpful due to the pretty-printing of conditions.

@rajveerappan
Copy link
Author

@tibbe / @tim-finnigan I finished up the PR and tests, please take a look: #3254

@tibbe
Copy link

tibbe commented Mar 8, 2023

@rajveerappan thanks for putting it together. Added a comment that I think is relevant at #3254 (comment)

@rajveerappan
Copy link
Author

@tim-finnigan how can we get this PR merged?

@rajveerappan
Copy link
Author

Hello @tim-finnigan, it looks like this issue overlaps with #4053 as well. I just rebased PR #3254 and pushed it up so it should be ready for re-review/merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dynamodb feature-request This issue requests a feature. needs-review p3 This is a minor priority issue resources
Projects
None yet
Development

No branches or pull requests

4 participants