-
Notifications
You must be signed in to change notification settings - Fork 867
Support ScannedCount on DynamoDb Document Model #3751
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
base: development
Are you sure you want to change the base?
Support ScannedCount on DynamoDb Document Model #3751
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments are affect both the sync and async code paths.
@@ -18,6 +18,7 @@ | |||
#else | |||
#error Unknown platform constant - unable to set correct AssemblyDescription | |||
#endif | |||
[assembly: InternalsVisibleTo("AWSSDK.UnitTests.DynamoDBv2.NetFramework, PublicKey=0024000004800000940000000602000000240000525341310004000001000100db5f59f098d27276c7833875a6263a3cc74ab17ba9a9df0b52aedbe7252745db7274d5271fd79c1f08f668ecfa8eaab5626fa76adc811d3c8fc55859b0d09d3bc0a84eecd0ba891f2b8a2fc55141cdcc37c2053d53491e650a479967c3622762977900eddbf1252ed08a2413f00a28f3a0752a81203f03ccb7f684db373518b4")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI this file is generated, it'll be overwritten by the generator the next time it runs.
I do see why you had to add it since a lot of the Search
related types are internal, but we may need a different approach here (e.g. would it be better to add an integration test instead? Or revisit the access modifiers to make them public?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dscpinheiro in this case is there any issue if I update the AssemblyInfo template and add assembly: InternalsVisibleTo
for unit tests projects which looks like respects naming convention AWSSDK.UnitTests.{AssemblyTitle}.NetFramework ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could work, but I think you'd also need to update the test projects to be signed... I'm honestly not a fan of making internals visible; I know we did that for Core
but I wish we didn't (which is why I mentioned maybe using an integration test instead if possible).
We can until Norm's back to get his opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @dscpinheiro and I'm okay with relying on InternalsVisibleTo
to make it easier to write unit tests. The generator will need to be updated to write this InternalsVisibleTo
since the AssemblyInfo.cs
is generated.
When doing the generator update you will need to add the logic to check if it is generating the DynamoDB AssemblyInfo
and if so add the attribute. I don't think we want to add this attribute to every service's AssemblyInfo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
templates and generator cod was updated for DynamoDB service projects. @normj please have another look.
@@ -18,6 +18,7 @@ | |||
#else | |||
#error Unknown platform constant - unable to set correct AssemblyDescription | |||
#endif | |||
[assembly: InternalsVisibleTo("AWSSDK.UnitTests.DynamoDBv2.NetFramework, PublicKey=0024000004800000940000000602000000240000525341310004000001000100db5f59f098d27276c7833875a6263a3cc74ab17ba9a9df0b52aedbe7252745db7274d5271fd79c1f08f668ecfa8eaab5626fa76adc811d3c8fc55859b0d09d3bc0a84eecd0ba891f2b8a2fc55141cdcc37c2053d53491e650a479967c3622762977900eddbf1252ed08a2413f00a28f3a0752a81203f03ccb7f684db373518b4")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @dscpinheiro and I'm okay with relying on InternalsVisibleTo
to make it easier to write unit tests. The generator will need to be updated to write this InternalsVisibleTo
since the AssemblyInfo.cs
is generated.
When doing the generator update you will need to add the logic to check if it is generating the DynamoDB AssemblyInfo
and if so add the attribute. I don't think we want to add this attribute to every service's AssemblyInfo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left minor comment and running the PR through the internal build system. Assuming that is successful I can approve.
generator/ServiceClientGeneratorLib/UnitTestProjectFileCreator.cs
Outdated
Show resolved
Hide resolved
Can you regenerate the Assembly.tt file so you get an updated Assembly.cs file? Looks like it got merged weird at one point in your branch and the version section is being generated without the In case you don't know what I mean in the Generator solution right click on the
|
I would also suggest avoiding commit the generated unit project file changes. That is how you will get conflicts like you have now. The generated changes will get picked and committed during the next release after this is merged. |
|
Not sure what happen with my previous internal build run that triggered the issues with AssemblyInfo. I assume my local branch was in a bad state and deleted and repulled. And I don't see the same problem. I push the branch through the internal build system again. You do need to address the merge conflict. That should just mean merge in the latest |
.NET Framework build is failing with the following build errors
|
added InternalsVisibleTo for AWSSDK.UnitTests.NetFramework to fix the build |
Expose
ScannedCount
on Amazon.DynamoDBv2.DocumentModel.SearchDescription
Add a new
ScannedCount
property to Search class.Populate this value internally from the most recent
ScanResponse
orQueryResponse
.Accumulate
ScannedCount
value forGetRemaining/GetRemainingAsync
Motivation and Context
#2018
Testing
Unit tests added for new property added
Screenshots (if appropriate)
Types of changes
Checklist
License