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

TEIIDSB 211 spring data s3 #267

Merged
merged 5 commits into from Jul 1, 2020

Conversation

aditya300899
Copy link
Member

No description provided.

Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

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

Looking good. I think we can go ahead and implement the encryption logic as well.

The encryption configuration property should default to AES256 - it should probably be renamed sseAlgorithm. Similarly encryptionKey should be re-named sseKey.

In S3VirtualFile s3Client.getObject(summary.getBucketName(), summary.getKey()); - can take a GetObjectRequest instead. If the key is set, you can then set that and the algorithm there.

In the existing implementation that's only on getFile. It must happen automatically on save on the server side.

Copy link
Member

@rareddy rareddy left a comment

Choose a reason for hiding this comment

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

Once this commited are we going to remove the current amazon-s3 or not?

@aditya300899
Copy link
Member Author

In the existing implementation that's only on getFile. It must happen automatically on save on the server side.

Like the getObject method takes a getObjectRequest, likewise the putObject also takes a putObjectRequest where we can set the key and corresponding algorithm!

I've added the functionality at both the places!

Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

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

Great. It makes more sense for it to be symmetrical.

@aditya300899
Copy link
Member Author

@shawkins I spent the day pretty much figuring out how to test using mockito. In the testGetFiles(), I was getting this error:
org.mockito.exceptions.misusing.MissingMethodInvocationException:
when() requires an argument which has to be 'a method call on a mock'.
For example:
when(mock.getArticles()).thenReturn(articles);

Also, this error might show up because:

  1. you stub either of: final/private/equals()/hashCode() methods.
    Those methods cannot be stubbed/verified.
    Mocking methods declared on non-public parent classes is not supported.
  2. inside when() you don't call method on mock but on some other object.

Can you help me, what I'm doing wrong?

And also, see that If I've structured the test well or it is supposed to be some other way?

@aditya300899
Copy link
Member Author

I've figured out the problem. I think it's good now. Suggest changes if any!!

Copy link
Member

@rareddy rareddy left a comment

Choose a reason for hiding this comment

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

lgtm

@shawkins
Copy link
Contributor

shawkins commented Jul 1, 2020

@aditya300899 were there any additional commits that you were going to make, such as minor cleanups of the tests like removing Assert.assertTrue(true), etc.? I can also do that with the porting to WildFly if you haven't already made those changes.

@aditya300899
Copy link
Member Author

I thought maybe you'd have more suggestions later, thus did not commit. Also, I was to think about the code coverage of a test by using some support in IntelliJ! I would do this later now.
As far as I remember and I'd noted down there were two changes:

  1. remove the AssertTrue(true)
  2. and not mocking the configuration
    I've included those changes in the commit above.

@shawkins shawkins merged commit de6df7a into teiid:master Jul 1, 2020
@aditya300899 aditya300899 deleted the TEIIDSB-211-spring-data-s3 branch July 8, 2020 07:05
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.

None yet

3 participants