Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
chibenwa commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1994648347 How about being clever and reworking `AttachmentId`? We could have: - An interface with a Factory for `AttachmentId` - Current AttachmentId class be renamed as` LegacyAttachmentId` and used by Cassandra app, JPA, memory - Have a new `UuidBackedAttachmentId` used in the Postgres app ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
hungphan227 closed pull request #2048: JAMES-2586 Fix some jmap postgres integration tests URL: https://github.com/apache/james-project/pull/2048 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
Arsnael commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1993147029 The JMAP integration test part seems to take often 40 ~ 50 minutes to complete which is too long (we consider less than 30 minutes ok) @hungphan227 Can we investigate again? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
Arsnael commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1990927785 Build was green (finally) I squashed and rebased -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
Arsnael commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1989922553 11:29:37,710 [INFO] Apache James :: Server :: JMAP RFC-8621 :: Postgres Integration Testing FAILURE [46:53 min] 46 mins... Maybe unstable CI or maybe not. Will restart the build to see -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
vttranlina commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1987633242 > Thanks. > > Can you test guys locally on those tests and make sure it passes with those fixes? => https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-2095/3/testReport/ - `webSocketShouldPushNewMessageWhenChangeSubscriptionOfMailbox` -> failed on my local. Related it, I discussed it with @hungphan227 on last Thursday, it failed because the test suite code, was not related to Postgres -> should update the test code - `checkShouldTurnFromDegradedToHealthyAfterMoreReadsOnAMissedProjection` -> failed - `shouldFailWithCannotCalculateChangesWhenSingleChangeIsTooLarge` -> passed -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
Arsnael commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1987619546 Thanks. Can you test guys locally on those tests and make sure it passes with those fixes? => https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-2095/3/testReport/ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
vttranlina commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1987593874 > fixup! JAMES-2586 Optimize AttachmentLoader Fixed https://github.com/apache/james-project/pull/2048/commits/144ac90c6483eda966301297afcf9c5cc73e55d6 Reason: Hanging when Collections is empty -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
Arsnael commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1985339243 @hungphan227 @vttranlina I'm going on my PR to test back without the last commit, could not fix it. Feel free to fix it yourselves if you want it back For example locally you can try to run `UidSearchOnIndex`, you will see quickly it's hanging forever -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
Arsnael commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1517293461 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresMessageIdMapper.java: ## @@ -130,18 +142,44 @@ public Publisher findMetadata(MessageId messageId @Override public Flux findReactive(Collection messageIds, MessageMapper.FetchType fetchType) { -return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), -fetchType) +return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), fetchType) .flatMap(messageBuilderAndRecord -> { SimpleMailboxMessage.Builder messageBuilder = messageBuilderAndRecord.getLeft(); +Record record = messageBuilderAndRecord.getRight(); if (fetchType == MessageMapper.FetchType.FULL) { -return retrieveFullContent(messageBuilderAndRecord.getRight()) -.map(headerAndBodyContent -> messageBuilder.content(headerAndBodyContent).build()); +return retrieveFullMessage(messageBuilder, record); } return Mono.just(messageBuilder.build()); }, ReactorUtils.DEFAULT_CONCURRENCY); } +private Mono retrieveFullMessage(SimpleMailboxMessage.Builder messageBuilder, Record record) { Review Comment: FYI that commit is what is making tests failing/hanging forever on imap mpt tests -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
Arsnael commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1517293461 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresMessageIdMapper.java: ## @@ -130,18 +142,44 @@ public Publisher findMetadata(MessageId messageId @Override public Flux findReactive(Collection messageIds, MessageMapper.FetchType fetchType) { -return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), -fetchType) +return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), fetchType) .flatMap(messageBuilderAndRecord -> { SimpleMailboxMessage.Builder messageBuilder = messageBuilderAndRecord.getLeft(); +Record record = messageBuilderAndRecord.getRight(); if (fetchType == MessageMapper.FetchType.FULL) { -return retrieveFullContent(messageBuilderAndRecord.getRight()) -.map(headerAndBodyContent -> messageBuilder.content(headerAndBodyContent).build()); +return retrieveFullMessage(messageBuilder, record); } return Mono.just(messageBuilder.build()); }, ReactorUtils.DEFAULT_CONCURRENCY); } +private Mono retrieveFullMessage(SimpleMailboxMessage.Builder messageBuilder, Record record) { Review Comment: FYI that commit is what is making tests failing on imap mpt tests -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
Arsnael commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1983105836 @quantranhong1999 => https://github.com/apache/james-project/pull/2095 My 50 cents, let's see :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
Arsnael commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1983038335 No I don't think so... I think I have a clue, and maybe we are a bit unlucky on some of our builds... let me try something and open an other PR, I noticed a pattern when such thing happens -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
quantranhong1999 commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1982954684 > I'm suspecting something being potentially unstable on our side as all the timeout builds have occured on different workers in the last few days Any chance it is related to this change: https://github.com/apache/james-project/pull/2072/files ... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
Arsnael commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1982919726 ``` 03:25:25.251 [INFO ] o.a.j.b.p.DockerPostgresSingleton - 2024-03-07 03:25:25.250 UTC [62] LOG: checkpoint starting: time 03:27:57.599 [INFO ] o.a.j.b.p.DockerPostgresSingleton - 2024-03-07 03:27:57.598 UTC [62] LOG: checkpoint complete: wrote 1524 buffers (9.3%); 0 WAL file(s) added, 0 removed, 1 recycled; write=152.270 s, sync=0.001 s, total=152.348 s; sync files=0, longest=0.000 s, average=0.000 s; distance=16207 kB, estimate=16207 kB; lsn=0/28E6CE0, redo lsn=0/28E6CA8 ``` Hanging again, same point... do we have an issue here in our pg test with the docker postgres signleton extension? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
vttranlina commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1980085869 > Related to the ci failed bodyStructureShouldSupportSpecificHeaders(GuiceJamesServer) we can cherry-pick fixup commit: https://github.com/apache/james-project/pull/2093 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
vttranlina commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1513805508 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresMessageIdMapper.java: ## @@ -130,18 +142,44 @@ public Publisher findMetadata(MessageId messageId @Override public Flux findReactive(Collection messageIds, MessageMapper.FetchType fetchType) { -return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), -fetchType) +return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), fetchType) .flatMap(messageBuilderAndRecord -> { SimpleMailboxMessage.Builder messageBuilder = messageBuilderAndRecord.getLeft(); +Record record = messageBuilderAndRecord.getRight(); if (fetchType == MessageMapper.FetchType.FULL) { -return retrieveFullContent(messageBuilderAndRecord.getRight()) -.map(headerAndBodyContent -> messageBuilder.content(headerAndBodyContent).build()); +return retrieveFullMessage(messageBuilder, record); } return Mono.just(messageBuilder.build()); }, ReactorUtils.DEFAULT_CONCURRENCY); } +private Mono retrieveFullMessage(SimpleMailboxMessage.Builder messageBuilder, Record record) { Review Comment: I don't have permission, you can fetch my code, and cherry-pick this commit -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
hungphan227 commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1513797407 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresMessageIdMapper.java: ## @@ -130,18 +142,44 @@ public Publisher findMetadata(MessageId messageId @Override public Flux findReactive(Collection messageIds, MessageMapper.FetchType fetchType) { -return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), -fetchType) +return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), fetchType) .flatMap(messageBuilderAndRecord -> { SimpleMailboxMessage.Builder messageBuilder = messageBuilderAndRecord.getLeft(); +Record record = messageBuilderAndRecord.getRight(); if (fetchType == MessageMapper.FetchType.FULL) { -return retrieveFullContent(messageBuilderAndRecord.getRight()) -.map(headerAndBodyContent -> messageBuilder.content(headerAndBodyContent).build()); +return retrieveFullMessage(messageBuilder, record); } return Mono.just(messageBuilder.build()); }, ReactorUtils.DEFAULT_CONCURRENCY); } +private Mono retrieveFullMessage(SimpleMailboxMessage.Builder messageBuilder, Record record) { Review Comment: great. tks. could you push it to this PR? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
vttranlina commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1980061620 Related to the ci failed ``` bodyStructureShouldSupportSpecificHeaders(GuiceJamesServer) – org.apache.james.jmap.rfc8621.postgres.PostgresEmailGetMethodTest JSON documents are different: Different value found in node "methodResponses[0][1].list[0].bodyStructure.blobId", expected: <"1_1"> but was: <"018e11c3-8f00-7d96-a046-4f8beef18834_1">. ``` I see we get the same in the Distributed test (master branch) ![image](https://github.com/apache/james-project/assets/81145350/d378ca91-c2be-4950-b665-59b06d382970) It looks like comes from test cases, not related to Postgres or not // I'm investigating it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
vttranlina commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1513787434 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresMessageIdMapper.java: ## @@ -130,18 +142,44 @@ public Publisher findMetadata(MessageId messageId @Override public Flux findReactive(Collection messageIds, MessageMapper.FetchType fetchType) { -return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), -fetchType) +return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), fetchType) .flatMap(messageBuilderAndRecord -> { SimpleMailboxMessage.Builder messageBuilder = messageBuilderAndRecord.getLeft(); +Record record = messageBuilderAndRecord.getRight(); if (fetchType == MessageMapper.FetchType.FULL) { -return retrieveFullContent(messageBuilderAndRecord.getRight()) -.map(headerAndBodyContent -> messageBuilder.content(headerAndBodyContent).build()); +return retrieveFullMessage(messageBuilder, record); } return Mono.just(messageBuilder.build()); }, ReactorUtils.DEFAULT_CONCURRENCY); } +private Mono retrieveFullMessage(SimpleMailboxMessage.Builder messageBuilder, Record record) { Review Comment: caught - Current `AttachmentLoader` get each by each `MessageAttachmentMetadata` by id - Your new code: get list MessageAttachmentMetadata by list id :+1: I have a refactor for that https://github.com/apache/james-project/commit/437550fd687ae4cbfd6986cbb712f2d940453061 can you look it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
vttranlina commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1513757850 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/dao/PostgresAttachmentDAO.java: ## @@ -80,9 +83,20 @@ public Mono> getAttachment(AttachmentId attachm blobIdFactory.from(row.get(PostgresAttachmentTable.BLOB_ID; } +public Flux getAttachments(Collection attachmentIds) { +return postgresExecutor.executeRows(dslContext -> Flux.from(dslContext.selectFrom(PostgresAttachmentTable.TABLE_NAME) Review Comment: fair enough -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
vttranlina commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1513738873 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresMessageIdMapper.java: ## @@ -130,18 +142,44 @@ public Publisher findMetadata(MessageId messageId @Override public Flux findReactive(Collection messageIds, MessageMapper.FetchType fetchType) { -return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), -fetchType) +return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), fetchType) .flatMap(messageBuilderAndRecord -> { SimpleMailboxMessage.Builder messageBuilder = messageBuilderAndRecord.getLeft(); +Record record = messageBuilderAndRecord.getRight(); if (fetchType == MessageMapper.FetchType.FULL) { -return retrieveFullContent(messageBuilderAndRecord.getRight()) -.map(headerAndBodyContent -> messageBuilder.content(headerAndBodyContent).build()); +return retrieveFullMessage(messageBuilder, record); } return Mono.just(messageBuilder.build()); }, ReactorUtils.DEFAULT_CONCURRENCY); } +private Mono retrieveFullMessage(SimpleMailboxMessage.Builder messageBuilder, Record record) { Review Comment: let me try it a bit -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
hungphan227 commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1513733102 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresMessageIdMapper.java: ## @@ -130,18 +142,44 @@ public Publisher findMetadata(MessageId messageId @Override public Flux findReactive(Collection messageIds, MessageMapper.FetchType fetchType) { -return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), -fetchType) +return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), fetchType) .flatMap(messageBuilderAndRecord -> { SimpleMailboxMessage.Builder messageBuilder = messageBuilderAndRecord.getLeft(); +Record record = messageBuilderAndRecord.getRight(); if (fetchType == MessageMapper.FetchType.FULL) { -return retrieveFullContent(messageBuilderAndRecord.getRight()) -.map(headerAndBodyContent -> messageBuilder.content(headerAndBodyContent).build()); +return retrieveFullMessage(messageBuilder, record); } return Mono.just(messageBuilder.build()); }, ReactorUtils.DEFAULT_CONCURRENCY); } +private Mono retrieveFullMessage(SimpleMailboxMessage.Builder messageBuilder, Record record) { Review Comment: I remember that AttachmentLoader's method does not fit this case -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
hungphan227 commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1513733102 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresMessageIdMapper.java: ## @@ -130,18 +142,44 @@ public Publisher findMetadata(MessageId messageId @Override public Flux findReactive(Collection messageIds, MessageMapper.FetchType fetchType) { -return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), -fetchType) +return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), fetchType) .flatMap(messageBuilderAndRecord -> { SimpleMailboxMessage.Builder messageBuilder = messageBuilderAndRecord.getLeft(); +Record record = messageBuilderAndRecord.getRight(); if (fetchType == MessageMapper.FetchType.FULL) { -return retrieveFullContent(messageBuilderAndRecord.getRight()) -.map(headerAndBodyContent -> messageBuilder.content(headerAndBodyContent).build()); +return retrieveFullMessage(messageBuilder, record); } return Mono.just(messageBuilder.build()); }, ReactorUtils.DEFAULT_CONCURRENCY); } +private Mono retrieveFullMessage(SimpleMailboxMessage.Builder messageBuilder, Record record) { Review Comment: I remember that AttachmentLoader's method that does not fit this case ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/dao/PostgresAttachmentDAO.java: ## @@ -80,9 +83,20 @@ public Mono> getAttachment(AttachmentId attachm blobIdFactory.from(row.get(PostgresAttachmentTable.BLOB_ID; } +public Flux getAttachments(Collection attachmentIds) { +return postgresExecutor.executeRows(dslContext -> Flux.from(dslContext.selectFrom(PostgresAttachmentTable.TABLE_NAME) Review Comment: I think the number of attachments of a single message is not so big that we have to partition IN list -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
vttranlina commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1512598925 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/dao/PostgresAttachmentDAO.java: ## @@ -80,9 +83,20 @@ public Mono> getAttachment(AttachmentId attachm blobIdFactory.from(row.get(PostgresAttachmentTable.BLOB_ID; } +public Flux getAttachments(Collection attachmentIds) { +return postgresExecutor.executeRows(dslContext -> Flux.from(dslContext.selectFrom(PostgresAttachmentTable.TABLE_NAME) Review Comment: As my above commented when using `IN` , we should partition it when `IN` size is large I don't see any reply about it? wdyt https://github.com/apache/james-project/pull/2048#discussion_r1502032559 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresMessageIdMapper.java: ## @@ -130,18 +142,44 @@ public Publisher findMetadata(MessageId messageId @Override public Flux findReactive(Collection messageIds, MessageMapper.FetchType fetchType) { -return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), -fetchType) +return mailboxMessageDAO.findMessagesByMessageIds(messageIds.stream().map(PostgresMessageId.class::cast).collect(ImmutableList.toImmutableList()), fetchType) .flatMap(messageBuilderAndRecord -> { SimpleMailboxMessage.Builder messageBuilder = messageBuilderAndRecord.getLeft(); +Record record = messageBuilderAndRecord.getRight(); if (fetchType == MessageMapper.FetchType.FULL) { -return retrieveFullContent(messageBuilderAndRecord.getRight()) -.map(headerAndBodyContent -> messageBuilder.content(headerAndBodyContent).build()); +return retrieveFullMessage(messageBuilder, record); } return Mono.just(messageBuilder.build()); }, ReactorUtils.DEFAULT_CONCURRENCY); } +private Mono retrieveFullMessage(SimpleMailboxMessage.Builder messageBuilder, Record record) { Review Comment: I see a lot of methods were written for adding attachment metadata to `SimpleMailboxMessage.Builder`. it is similar to what we did in [AttachmentLoader](https://github.com/apache/james-project/blob/3243752f862aa9cf4dff9474878837368f4442c4/mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/AttachmentLoader.java) Can we reuse it? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
Arsnael commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1975924591 Please rebase -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
quantranhong1999 commented on PR #2048: URL: https://github.com/apache/james-project/pull/2048#issuecomment-1970391358 org.apache.james.events.PostgresEventDeadLettersTest Related? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
hungphan227 commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1502284797 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/dao/PostgresAttachmentDAO.java: ## @@ -80,9 +84,21 @@ public Mono> getAttachment(AttachmentId attachm blobIdFactory.from(row.get(PostgresAttachmentTable.BLOB_ID; } +public Flux getAttachments(Collection attachmentIds) { +return postgresExecutor.executeRows(dslContext -> Flux.from(dslContext.selectFrom(PostgresAttachmentTable.TABLE_NAME) + .where(PostgresAttachmentTable.ID_AS_UUID.in(attachmentIds.stream().map(AttachmentId::asUUID).collect(ImmutableList.toImmutableList()) Review Comment: Removed ID_AS_UUID. Use ID as pk (change type from UUID to VARCHAR) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
hungphan227 commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1502284352 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresAttachmentModule.java: ## @@ -35,20 +35,22 @@ public interface PostgresAttachmentModule { interface PostgresAttachmentTable { Table TABLE_NAME = DSL.table("attachment"); -Field ID = DSL.field("id", SQLDataType.UUID.notNull()); +Field ID_AS_UUID = DSL.field("id_as_uuid", SQLDataType.UUID.notNull()); +Field ID = DSL.field("id", SQLDataType.VARCHAR.notNull()); Review Comment: Removed ID_AS_UUID. Use ID as pk (change type from UUID to VARCHAR) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
vttranlina commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1502211561 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/dao/PostgresAttachmentDAO.java: ## @@ -80,9 +84,21 @@ public Mono> getAttachment(AttachmentId attachm blobIdFactory.from(row.get(PostgresAttachmentTable.BLOB_ID; } +public Flux getAttachments(Collection attachmentIds) { +return postgresExecutor.executeRows(dslContext -> Flux.from(dslContext.selectFrom(PostgresAttachmentTable.TABLE_NAME) + .where(PostgresAttachmentTable.ID_AS_UUID.in(attachmentIds.stream().map(AttachmentId::asUUID).collect(ImmutableList.toImmutableList()) Review Comment: (1) updated Understood why you need ID and ID_AS_UUID I think we can use a single column for it. (`ID`, maybe. And change type from UUID -> VARCHAR) it is enough. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
vttranlina commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1502032559 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/dao/PostgresAttachmentDAO.java: ## @@ -80,9 +84,21 @@ public Mono> getAttachment(AttachmentId attachm blobIdFactory.from(row.get(PostgresAttachmentTable.BLOB_ID; } +public Flux getAttachments(Collection attachmentIds) { +return postgresExecutor.executeRows(dslContext -> Flux.from(dslContext.selectFrom(PostgresAttachmentTable.TABLE_NAME) + .where(PostgresAttachmentTable.ID_AS_UUID.in(attachmentIds.stream().map(AttachmentId::asUUID).collect(ImmutableList.toImmutableList()) Review Comment: 1. Why do we don't use directly "AttachmentId.getId()" in `where` clause? 2. When using "IN" clause, we should check the size of IN, if large, we should partition it, see the same `PostgresMailboxMessageDAO#findMessagesByMailboxIdAndUIDs` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
hungphan227 commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1502017082 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresAttachmentModule.java: ## @@ -35,20 +35,22 @@ public interface PostgresAttachmentModule { interface PostgresAttachmentTable { Table TABLE_NAME = DSL.table("attachment"); -Field ID = DSL.field("id", SQLDataType.UUID.notNull()); +Field ID_AS_UUID = DSL.field("id_as_uuid", SQLDataType.UUID.notNull()); +Field ID = DSL.field("id", SQLDataType.VARCHAR.notNull()); Review Comment: No, this is not "just to pass IT". In mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/dao/PostgresAttachmentDAO.java line 87: ``` public Flux getAttachments(Collection attachmentIds) { return postgresExecutor.executeRows(dslContext -> Flux.from(dslContext.selectFrom(PostgresAttachmentTable.TABLE_NAME) .where(PostgresAttachmentTable.ID_AS_UUID.in(attachmentIds.stream().map(AttachmentId::asUUID).collect(ImmutableList.toImmutableList()) .map(row -> AttachmentMetadata.builder() .attachmentId(AttachmentId.from(row.get(PostgresAttachmentTable.ID))) .type(row.get(PostgresAttachmentTable.TYPE)) .messageId(PostgresMessageId.Factory.of(row.get(PostgresAttachmentTable.MESSAGE_ID))) .size(row.get(PostgresAttachmentTable.SIZE)) .build()); } ``` If we do not have ID_AS_UUID, we have to use a hashmap to save AttachmentId - UUID (UUID.nameUUIDFromBytes(id.getBytes(StandardCharsets.UTF_8))) which is not good option IMO -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
Arsnael commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1502013047 ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresAttachmentModule.java: ## @@ -35,20 +35,22 @@ public interface PostgresAttachmentModule { interface PostgresAttachmentTable { Table TABLE_NAME = DSL.table("attachment"); -Field ID = DSL.field("id", SQLDataType.UUID.notNull()); +Field ID_AS_UUID = DSL.field("id_as_uuid", SQLDataType.UUID.notNull()); +Field ID = DSL.field("id", SQLDataType.VARCHAR.notNull()); Review Comment: If the tests fails because the structure is different, I would prefer here that we duplicate the test suite and adapt it to postgres attachment table structure instead? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
hungphan227 commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1502005171 ## server/protocols/jmap-rfc-8621-integration-tests/postgres-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/postgres/PostgresThreadGetTest.java: ## @@ -88,31 +60,11 @@ public class PostgresThreadGetTest extends PostgresBase implements ThreadGetCont .overrideWith(new TestJMAPServerModule())) .build(); -@AfterEach -void tearDown() throws IOException { -client.close(); -} - @Override public void awaitMessageCount(List mailboxIds, SearchQuery query, long messageCount) { -awaitForOpenSearch(queryConverter.from(mailboxIds, query), messageCount); } @Override public void initOpenSearchClient() { -client = MailboxIndexCreationUtil.prepareDefaultClient( -openSearch.getDockerOpenSearch().clientProvider().get(), -openSearch.getDockerOpenSearch().configuration()); -} - -private void awaitForOpenSearch(Query query, long totalHits) { Review Comment: In postgres implementation, Thread feature use postgres instead of opensearch -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-2586 Fix some jmap postgres integration tests [james-project]
quantranhong1999 commented on code in PR #2048: URL: https://github.com/apache/james-project/pull/2048#discussion_r1501987659 ## server/protocols/jmap-rfc-8621-integration-tests/postgres-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/postgres/PostgresThreadGetTest.java: ## @@ -88,31 +60,11 @@ public class PostgresThreadGetTest extends PostgresBase implements ThreadGetCont .overrideWith(new TestJMAPServerModule())) .build(); -@AfterEach -void tearDown() throws IOException { -client.close(); -} - @Override public void awaitMessageCount(List mailboxIds, SearchQuery query, long messageCount) { -awaitForOpenSearch(queryConverter.from(mailboxIds, query), messageCount); } @Override public void initOpenSearchClient() { -client = MailboxIndexCreationUtil.prepareDefaultClient( -openSearch.getDockerOpenSearch().clientProvider().get(), -openSearch.getDockerOpenSearch().configuration()); -} - -private void awaitForOpenSearch(Query query, long totalHits) { Review Comment: Why remove this? ## mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresAttachmentModule.java: ## @@ -35,20 +35,22 @@ public interface PostgresAttachmentModule { interface PostgresAttachmentTable { Table TABLE_NAME = DSL.table("attachment"); -Field ID = DSL.field("id", SQLDataType.UUID.notNull()); +Field ID_AS_UUID = DSL.field("id_as_uuid", SQLDataType.UUID.notNull()); +Field ID = DSL.field("id", SQLDataType.VARCHAR.notNull()); Review Comment: So we need the "redundant" ID field just to pass IT? Any other way? ## server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailQueryMethodContract.scala: ## @@ -7034,22 +7034,24 @@ trait EmailQueryMethodContract { |"c1"]] |}""".stripMargin -val response = `given` - .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER) - .body(request) -.when - .post -.`then` - .statusCode(SC_OK) - .contentType(JSON) - .extract - .body - .asString +awaitAtMostTenSeconds.untilAsserted { () => + val response = `given` +.header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER) +.body(request) +.when +.post +.`then` Review Comment: indent -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org