feat(CO-727): allow Save To Files from a shared mailbox #255

frisonisland · 2023-06-16T13:54:08Z

This PR expands the Save-To-Files by enabling the feature from a shared mailbox.

Changes:

Allow save to files with mid UUID:id, where UUID is other account UUID, id is message id
Add MockServer (https://www.mock-server.com/) to mock Files (this spots runtime exception on files-client in case of different dependencies version)
Update httpmime 4.5.13 -> 4.5.14 (https://mvnrepository.com/artifact/org.apache.httpcomponents/httpmime)
Update httpcore 4.4.14 -> 4.4.16 (https://mvnrepository.com/artifact/org.apache.httpcomponents/httpcore)
Add test for shared mailbox
Add parameterized test for different attachment (mime can have different behavior in size)
Use Try.withResources to close the stream when used (this was missing in previous implementation)
Add INTERNAL_ERROR in ServiceException

- add test to check shared mailbox is used to search for attachment - split messageId in UUID:messageId

- make test variables more idiomatic

- add map instead of flatMap to avoid nullpointer exception in case of null response from Files client - Cannot use latest mock server version because of jackson compatibility - Tests fail with missing com/fasterxml/jackson/databind/DeserializationFeature.FAIL_ON_TRAILING_TOKENS with latest version of mock server

- remove verify method call test since there is already a similar test - simplify implementation using vavr For - use vavr try withresources

- use should-when syntax

Using mockserver 5.13.0+ is not possible for jackson compatibility issues.

keshavbhatt

I made few suggestions to improve code quality.

Please use recoverWith instead of mapFailure (as it adds additional overhead in some places where not required) in other places as well where possible. I just pointed out once instance, but saw usage in other places.

store/src/test/java/com/zimbra/cs/service/mail/CopyToFilesIT.java

store/src/main/java/com/zimbra/cs/service/mail/CopyToFiles.java

* JUnit5 does not require methods to be public

* Use recoverWith to improve readability * use mapFailure when needed (multiple cases) * Add ServiceException "internal error" * Add more tests on possible failures of CopyToFiles

store/src/main/java/com/zimbra/cs/service/mail/CopyToFiles.java

keshavbhatt

LGTM.

I don't recommend the current exception propagation approach used in this codebase because it makes it difficult to track them in the calling function. I think it can be suitable in certain scenarios where the exceptions are not critical and can be disregarded.

Additionally, I suggest avoiding excessive use of functional chaining as it makes the code appear messy and unorganized.

frisonisland · 2023-06-20T08:46:39Z

LGTM.

I don't recommend the current exception propagation approach used in this codebase because it makes it difficult to track them in the calling function. I think it can be suitable in certain scenarios where the exceptions are not critical and can be disregarded.

Additionally, I suggest avoiding excessive use of functional chaining as it makes the code appear messy and unorganized.

I see this class as a Controller, that's why I decided to map the failure on each validation step (check token, check body, check messageId, check file exists, etc.). Then all is chained in the handle method
Unfortunately about functional I cannot say much, some people like it more, some not (aside other technical benefits).
This is also how vavr works.
I think we should opt for a common decision that works for us.

- Fixup check of exception message

Polpetta · 2023-06-22T07:42:56Z

Additionally, I suggest avoiding excessive use of functional chaining as it makes the code appear messy and unorganized.

Sorry, but I'm completely opposite to this statement. Please do use more functional chaining as it makes the code more maintainable by making it declarative

Functional seems scary at the beginning, but once it enters in your mind and you understand how it works it is a breeze to your programming experience.

keshavbhatt · 2023-06-22T08:01:55Z

Sorry, but I'm completely opposite to this statement.

I dont have issues with employing funtional apparoach to write the code. The issue I was trying to point out is "excessive use of functional chaining" that we can see in this code. That is making the code hard to debug and read.

"The code appear messy and unorganized"

I mean ⤵️

  final NodeId nodeId =
       Optional.ofNullable(
               getRequestObject(request)
                   .flatMap(
                       copyToFilesRequest ->
                           this.getAttachmentToCopy(copyToFilesRequest, zsc)
                               .flatMap(
                                   attachment ->
                                       Try.withResources(attachment::getInputStream)
                                           .of(
                                               inputStream ->
                                                   For(
                                                           Try.of(
                                                               () ->
                                                                   ZimbraCookie
                                                                           .COOKIE_ZM_AUTH_TOKEN
                                                                       + "="
                                                                       + zsc.getAuthToken()
                                                                           .getEncoded()),
                                                           this.getDestinationFolderId(
                                                               copyToFilesRequest),
                                                           this.getAttachmentName(attachment),
                                                           this.getAttachmentContentType(
                                                               attachment),
                                                           Try.withResources(
                                                                   attachment::getInputStream)
                                                               .of(this::getAttachmentSize)
                                                               .flatMap(identity()))
                                                       .yield(
                                                           (cookie,
                                                               folderId,
                                                               fileName,
                                                               contentType,
                                                               fileSize) ->
                                                               filesClient.uploadFile(
                                                                   cookie,
                                                                   folderId,
                                                                   fileName,
                                                                   contentType,
                                                                   inputStream,
                                                                   fileSize)))))
                   .flatMap(identity())
                   .flatMap(identity())
                   .mapFailure(
                       Case(
                           $(ex -> !(ex instanceof ServiceException)),
                           ServiceException::INTERNAL_ERROR))
                   .get())
           .orElseThrow(() -> ServiceException.FAILURE("got null response from Files server."));

frisonisland · 2023-06-22T08:05:21Z

Sorry, but I'm completely opposite to this statement.

I dont have issues with employing funtional apparoach to write the code. The issue I was trying to point out is "excessive use of functional chaining" that we can see in this code. That is making the code hard to debug and read.

"The code appear messy and unorganized"

I mean arrow_heading_down

  final NodeId nodeId =
       Optional.ofNullable(
               getRequestObject(request)
                   .flatMap(
                       copyToFilesRequest ->
                           this.getAttachmentToCopy(copyToFilesRequest, zsc)
                               .flatMap(
                                   attachment ->
                                       Try.withResources(attachment::getInputStream)
                                           .of(
                                               inputStream ->
                                                   For(
                                                           Try.of(
                                                               () ->
                                                                   ZimbraCookie
                                                                           .COOKIE_ZM_AUTH_TOKEN
                                                                       + "="
                                                                       + zsc.getAuthToken()
                                                                           .getEncoded()),
                                                           this.getDestinationFolderId(
                                                               copyToFilesRequest),
                                                           this.getAttachmentName(attachment),
                                                           this.getAttachmentContentType(
                                                               attachment),
                                                           Try.withResources(
                                                                   attachment::getInputStream)
                                                               .of(this::getAttachmentSize)
                                                               .flatMap(identity()))
                                                       .yield(
                                                           (cookie,
                                                               folderId,
                                                               fileName,
                                                               contentType,
                                                               fileSize) ->
                                                               filesClient.uploadFile(
                                                                   cookie,
                                                                   folderId,
                                                                   fileName,
                                                                   contentType,
                                                                   inputStream,
                                                                   fileSize)))))
                   .flatMap(identity())
                   .flatMap(identity())
                   .mapFailure(
                       Case(
                           $(ex -> !(ex instanceof ServiceException)),
                           ServiceException::INTERNAL_ERROR))
                   .get())
           .orElseThrow(() -> ServiceException.FAILURE("got null response from Files server."));

TBH it's just the "For" that makes the code messy, but the rest is quite straightforward

frisonisland · 2023-06-22T08:16:33Z

"The code appear messy and unorganized"

I mean arrow_heading_down

  final NodeId nodeId =
       Optional.ofNullable(
               getRequestObject(request)
                   .flatMap(
                       copyToFilesRequest ->
                           this.getAttachmentToCopy(copyToFilesRequest, zsc)
                               .flatMap(
                                   attachment ->
                                       Try.withResources(attachment::getInputStream)
                                           .of(
                                               inputStream ->
                                                   For(
                                                           Try.of(
                                                               () ->
                                                                   ZimbraCookie
                                                                           .COOKIE_ZM_AUTH_TOKEN
                                                                       + "="
                                                                       + zsc.getAuthToken()
                                                                           .getEncoded()),
                                                           this.getDestinationFolderId(
                                                               copyToFilesRequest),
                                                           this.getAttachmentName(attachment),
                                                           this.getAttachmentContentType(
                                                               attachment),
                                                           Try.withResources(
                                                                   attachment::getInputStream)
                                                               .of(this::getAttachmentSize)
                                                               .flatMap(identity()))
                                                       .yield(
                                                           (cookie,
                                                               folderId,
                                                               fileName,
                                                               contentType,
                                                               fileSize) ->
                                                               filesClient.uploadFile(
                                                                   cookie,
                                                                   folderId,
                                                                   fileName,
                                                                   contentType,
                                                                   inputStream,
                                                                   fileSize)))))
                   .flatMap(identity())
                   .flatMap(identity())
                   .mapFailure(
                       Case(
                           $(ex -> !(ex instanceof ServiceException)),
                           ServiceException::INTERNAL_ERROR))
                   .get())
           .orElseThrow(() -> ServiceException.FAILURE("got null response from Files server."));

@keshav I can do the following (introducing a method to get the cookie):

Optional.ofNullable(
                getRequestObject(request)
                    .flatMap(
                        copyToFilesRequest ->
                            this.getAttachmentToCopy(copyToFilesRequest, zsc)
                                .flatMap(
                                    attachment ->
                                        Try.withResources(attachment::getInputStream)
                                            .of(
                                                inputStream ->
                                                    For(this.getCookie(zsc),
                                                            this.getDestinationFolderId(copyToFilesRequest),
                                                            this.getAttachmentName(attachment),
                                                            this.getAttachmentContentType(attachment),
                                                            Try.of(() -> inputStream),
                                                            Try.withResources(
                                                                    attachment::getInputStream)
                                                                .of(this::getAttachmentSize)
                                                                .flatMap(identity()))
                                                        .yield(filesClient::uploadFile))))
                    .flatMap(identity())
                    .flatMap(identity())
                    .onFailure(ex -> mLog.error(ex.getMessage()))
                    .mapFailure(
                        Case(
                            $(ex -> !(ex instanceof ServiceException)),
                            ServiceException::INTERNAL_ERROR))
                    .get())
                    .orElseThrow(() -> ServiceException.FAILURE("got null response from Files server."));

Edit: there also other ways, I can change a bit some of the private methods. Initially they were different, I can use the try with resources in private methods for example. I understand your point, the handle method should be as clean as possible so to understand the flow.

- Refactor CopyToFiles to make handle method more readable - Log error instead of debug when API fails

frisonisland · 2023-06-22T08:30:51Z

Made changes about flow and added Log with ERROR when API fails. Check again

Polpetta · 2023-06-22T09:12:49Z

The problem is not the functional approach, is that there are a lot of objects to unwrap. This is kinda a bad design for whom implemented it in the past, little we can do with such a codebase other than trying our best.
The getCookie helps a little I agree

store/src/main/java/com/zimbra/cs/service/mail/CopyToFiles.java

store/src/test/resources/test-save-to-files.jpg

Polpetta · 2023-06-22T09:48:01Z

Also I saw that you edited stuff related to certbot in ProxtConfGen, like deleteDomainCertbotConfiguration. Why?

- Image from: https://www.pexels.com/photo/aerial-photography-of-cloudy-mountain-7919/

frisonisland · 2023-06-26T10:13:01Z

Also I saw that you edited stuff related to certbot in ProxtConfGen, like deleteDomainCertbotConfiguration. Why?

Check again, there are no changes on ProxyConfGen!

- Use Guava not(instanceOf) instead of !( -> instanceOf)

Polpetta

LGTM

frisonisland added 16 commits June 6, 2023 15:59

feat: add copy to files with shared mailbox

c8ac979

- add test to check shared mailbox is used to search for attachment - split messageId in UUID:messageId

chore: small changes in variable

5e4e286

- make test variables more idiomatic

Merge remote-tracking branch 'origin/devel' into feat/CO-727

4793e5b

chore: use unknown length

380239a

Merge remote-tracking branch 'origin/devel' into feat/CO-727

f67ef99

chore: use save draft + check response

e8cdced

chore: added test for delegated account with grant options

756600c

chore: simplify copytofiles vavr + try-with-resources

69e653e

- remove verify method call test since there is already a similar test - simplify implementation using vavr For - use vavr try withresources

chore: rename some tests

5cddd4a

- use should-when syntax

chore: save only

fe1ade6

chore: merge from devel + adapt to junit 5

be91f8d

chore: merge from devel + adapt to junit 5 (2)

1464d96

chore: remove junit4 import

564ed76

chore: use mockserver 5.12.0

2efe36d

Using mockserver 5.13.0+ is not possible for jackson compatibility issues.

chore: use mockserver 5.13.0

26a3b75

frisonisland requested review from keshavbhatt and Polpetta and removed request for keshavbhatt June 19, 2023 08:14

keshavbhatt requested changes Jun 19, 2023

View reviewed changes

frisonisland added 3 commits June 19, 2023 14:01

chore: CopyToFilesIT package private

617f09e

* JUnit5 does not require methods to be public

chore: clean up with suggestions from PR

b15e000

* Use recoverWith to improve readability * use mapFailure when needed (multiple cases) * Add ServiceException "internal error" * Add more tests on possible failures of CopyToFiles

chore: blank diamond operator

d0f241c

frisonisland requested a review from keshavbhatt June 19, 2023 13:56

keshavbhatt reviewed Jun 20, 2023

View reviewed changes

store/src/main/java/com/zimbra/cs/service/mail/CopyToFiles.java Show resolved Hide resolved

keshavbhatt approved these changes Jun 20, 2023

View reviewed changes

frisonisland added 2 commits June 20, 2023 11:47

chore: assert exception message in test

cab0cad

- Fixup check of exception message

chore: log with error

2016c65

frisonisland added 2 commits June 22, 2023 10:28

chore: cleanup CopyToFiles handle + log error

abd5566

- Refactor CopyToFiles to make handle method more readable - Log error instead of debug when API fails

chore: remove test added by mistake

aaf4306

frisonisland requested a review from keshavbhatt June 22, 2023 08:31

Polpetta reviewed Jun 22, 2023

View reviewed changes

store/src/main/java/com/zimbra/cs/service/mail/CopyToFiles.java Outdated Show resolved Hide resolved

store/src/test/resources/test-save-to-files.jpg Outdated Show resolved Hide resolved

chore: add CC0 image for testing

28b4dd9

- Image from: https://www.pexels.com/photo/aerial-photography-of-cloudy-mountain-7919/

frisonisland requested a review from a team as a code owner June 26, 2023 10:11

frisonisland requested review from aheeva-yuliya and removed request for a team June 26, 2023 10:11

chore: use predicate not

f5a5706

- Use Guava not(instanceOf) instead of !( -> instanceOf)

keshavbhatt approved these changes Jun 26, 2023

View reviewed changes

Polpetta approved these changes Jun 26, 2023

View reviewed changes

frisonisland merged commit 052d460 into devel Jun 26, 2023

frisonisland deleted the feat/CO-727 branch June 26, 2023 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(CO-727): allow Save To Files from a shared mailbox #255

feat(CO-727): allow Save To Files from a shared mailbox #255

frisonisland commented Jun 16, 2023 •

edited

Loading

keshavbhatt left a comment

keshavbhatt left a comment

frisonisland commented Jun 20, 2023

Polpetta commented Jun 22, 2023

keshavbhatt commented Jun 22, 2023

frisonisland commented Jun 22, 2023

frisonisland commented Jun 22, 2023 •

edited

Loading

frisonisland commented Jun 22, 2023

Polpetta commented Jun 22, 2023

Polpetta commented Jun 22, 2023

frisonisland commented Jun 26, 2023

Polpetta left a comment

feat(CO-727): allow Save To Files from a shared mailbox #255

feat(CO-727): allow Save To Files from a shared mailbox #255

Conversation

frisonisland commented Jun 16, 2023 • edited Loading

keshavbhatt left a comment

Choose a reason for hiding this comment

keshavbhatt left a comment

Choose a reason for hiding this comment

frisonisland commented Jun 20, 2023

Polpetta commented Jun 22, 2023

keshavbhatt commented Jun 22, 2023

frisonisland commented Jun 22, 2023

frisonisland commented Jun 22, 2023 • edited Loading

frisonisland commented Jun 22, 2023

Polpetta commented Jun 22, 2023

Polpetta commented Jun 22, 2023

frisonisland commented Jun 26, 2023

Polpetta left a comment

Choose a reason for hiding this comment

frisonisland commented Jun 16, 2023 •

edited

Loading

frisonisland commented Jun 22, 2023 •

edited

Loading