search

mboudreau / Alternator

A mock DynamoDB that runs locally for testing purposes - DEPRECATED, PLEASE USE DYNAMODB LOCAL: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/DynamoDBLocal.html
Apache License 2.0
78 stars 39 forks source link

Fix for Unmarshaller changes in aws 1.10.5.1 #96

Closed jentfoo closed 9 years ago

jentfoo commented 9 years ago

I am unsure what this change would impact for use with older aws libraries. This appears to fix the library running with aws 1.10.5.1 for me.

If you do accept this PR would you please publish a new artifact for me as well.

mboudreau commented 9 years ago

@jentfoo So, if I understand correctly, you've removed all of the marshallers, excluding the JSON marshaller, and expect that the library will behave as it should?

@rrutt Can you comment on this? I figured the marshallers were needed to be able to parse certain requests/responses. Wouldn't this break functionality?

jentfoo commented 9 years ago

They were removed inside the latest AWS library. I did not see an obvious replacement, so I attempted to remove them. It appears to still work fine in my unit tests, and I assumed the unit tests built into the library, so I thought this may be okay. But yea, I definetly was not sure.

mboudreau commented 9 years ago

Ah good, yeah, I just checked the AWS SDK sourcecode and they seem to be gone. Let's just wait for what Rick has to say since he would have a better understanding to see if we might be using these marshallers anywhere else or if it might affect the system.

If not, I'll merge and create a new artifact :)

On Thu, Jul 30, 2015 at 10:54 AM Mike Jensen notifications@github.com wrote:

They were removed inside the latest AWS library. I did not see an obvious replacement, so I attempted to remove them. It appears to still work fine in my unit tests, and I assumed the unit tests built into the library, so I thought this may be okay. But yea, I definetly was not sure.

— Reply to this email directly or view it on GitHub https://github.com/mboudreau/Alternator/pull/96#issuecomment-126143639.

rrutt commented 9 years ago

I will clone Mike Jenson's repo, checkout his aws-1.10.5.1 branch, and run the JUnit tests. I run them three different ways by temporarily editing constant values in the AlternatorTest base classes (both v1 and v2). This tests normally (self-launched service), in-process, and with the Alternator jar running as an external process.

The constant values involved are RUN_DB_AS_SERVICE and SPAWN_LOCAL_DB_SERVICE. Given two Booleans, 4 combinations are possible but only 3 make sense.

I will let you know the test results.

mboudreau commented 9 years ago

Amazing, thank you.

On Thu, Jul 30, 2015 at 10:43 PM Rick Rutt notifications@github.com wrote:

I will clone Mike Jenson's repo, checkout his aws-1.10.5.1 branch, and run the JUnit tests. I run them three different ways by temporarily editing constant values in the AlternatorTest base classes (both v1 and v2). This tests normally (self-launched service), in-process, and with the Alternator jar running as an external process.

The constant values involved are RUN_DB_AS_SERVICE and SPAWN_LOCAL_DB_SERVICE. Given two Booleans, 4 combinations are possible but only 3 make sense.

I will let you know the test results.

— Reply to this email directly or view it on GitHub https://github.com/mboudreau/Alternator/pull/96#issuecomment-126310043.

rrutt commented 9 years ago

I was able to run the JUnit tests and confirm that Mike Jenson's revisions do not break anything.

However, the AWS SDK for DynamoDB in this version has slightly changed its exception behavior for certain operations. Several operations now throw AmazonServiceException instead of a specific exception. The .getErrorCode() for the AmazonServiceException is the string name of the original specific exception.

I have a followup Pull Request based on this one that updates the JUnit tests to adapt to this change.

I recommend you merge this current Pull Request from Mike Jensen.

My followup Pull Request only affects developers that clone the Alternator source code to compile on their own workstations. The JUnit test changes are technically not required for a Nexus binary release.

Nevertheless, I will be submitting my Pull Request in just a few moments.

jentfoo commented 9 years ago

Thank you! can you let me know once you have deployed an artifact with these changes?

mboudreau commented 9 years ago

Yep, going to deploy in the next hour.

On Fri, Jul 31, 2015, 8:39 AM Mike Jensen notifications@github.com wrote:

Thank you! can you let me know once you have deployed an artifact with these changes?

— Reply to this email directly or view it on GitHub https://github.com/mboudreau/Alternator/pull/96#issuecomment-126511238.

mboudreau commented 9 years ago

Sorry this took a while, today has been hectic. version 0.12.0 has been deployed.

On Fri, Jul 31, 2015 at 9:04 AM Michel Boudreau michelboudreau@gmail.com wrote:

Yep, going to deploy in the next hour.

On Fri, Jul 31, 2015, 8:39 AM Mike Jensen notifications@github.com wrote:

Thank you! can you let me know once you have deployed an artifact with these changes?

— Reply to this email directly or view it on GitHub https://github.com/mboudreau/Alternator/pull/96#issuecomment-126511238.

jentfoo commented 9 years ago

Thanks for the new artifact. It looks great, I also confirmed it works with the version of AWS that was released yesterday (1.10.8), so no issues there (yay!).