You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2021/04/03 07:16:48 UTC
[GitHub] [james-project] chibenwa opened a new pull request #358: [REFACTORING] JMAP: All ids should be backed by IdConstraint
chibenwa opened a new pull request #358:
URL: https://github.com/apache/james-project/pull/358
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] mbaechler commented on a change in pull request #358: [REFACTORING] JMAP: All ids should be backed by IdConstraint
Posted by GitBox <gi...@apache.org>.
mbaechler commented on a change in pull request #358:
URL: https://github.com/apache/james-project/pull/358#discussion_r607556639
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxSet.scala
##########
@@ -46,7 +47,7 @@ case class MailboxSetRequest(accountId: AccountId,
onDestroyRemoveEmails: Option[RemoveEmailsOnDestroy]) extends WithAccountId
object MailboxSetRequest {
- type MailboxCreationId = String Refined NonEmpty
+ type MailboxCreationId = String Refined IdConstraint
Review comment:
you are right, it's already an enhancement.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] mbaechler commented on a change in pull request #358: [REFACTORING] JMAP: All ids should be backed by IdConstraint
Posted by GitBox <gi...@apache.org>.
mbaechler commented on a change in pull request #358:
URL: https://github.com/apache/james-project/pull/358#discussion_r607557358
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala
##########
@@ -1059,7 +1059,7 @@ trait MailboxSetMethodContract {
| "notCreated": {
| "C42": {
| "type": "invalidArguments",
- | "description": "'/parentId' property in mailbox object is not valid: Predicate isEmpty() did not fail."
+ | "description": "'/parentId' property in mailbox object is not valid: Left predicate of ((!(0 < 1) && !(0 > 255)) && \\"\\".matches(\\"^[#a-zA-Z0-9-_]*$$\\")) failed: Predicate taking size() = 0 failed: Left predicate of (!(0 < 1) && !(0 > 255)) failed: Predicate (0 < 1) did not fail."
Review comment:
It's not the message that I dislike, it's the assertion, sorry if my comment was confusing. Could you just ignore part of this message?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #358: [REFACTORING] JMAP: All ids should be backed by IdConstraint
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #358:
URL: https://github.com/apache/james-project/pull/358#discussion_r607434206
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxSet.scala
##########
@@ -46,7 +47,7 @@ case class MailboxSetRequest(accountId: AccountId,
onDestroyRemoveEmails: Option[RemoveEmailsOnDestroy]) extends WithAccountId
object MailboxSetRequest {
- type MailboxCreationId = String Refined NonEmpty
+ type MailboxCreationId = String Refined IdConstraint
Review comment:
Fair. But it is not a regression, as before everything was 'refined NonEmpty'
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #358: [REFACTORING] JMAP: All ids should be backed by IdConstraint
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #358:
URL: https://github.com/apache/james-project/pull/358#discussion_r606941993
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/VacationResponseGetMethodContract.scala
##########
@@ -482,14 +482,16 @@ trait VacationResponseGetMethodContract {
.asString
.stripMargin
- assertThatJson(response).isEqualTo(
+ assertThatJson(response)
+ .whenIgnoringPaths("methodResponses[0][1].description")
Review comment:
Honestly I gave up trying to understand the escape sequence after 1 hour +.
(a Json in a JSON in a scala string).
I see little value in spending my time doing it.
If you wonna give it a shot though, contribs are welcome.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on a change in pull request #358: [REFACTORING] JMAP: All ids should be backed by IdConstraint
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #358:
URL: https://github.com/apache/james-project/pull/358#discussion_r606957534
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/VacationResponseGetMethodContract.scala
##########
@@ -482,14 +482,16 @@ trait VacationResponseGetMethodContract {
.asString
.stripMargin
- assertThatJson(response).isEqualTo(
+ assertThatJson(response)
+ .whenIgnoringPaths("methodResponses[0][1].description")
Review comment:
It just feels pointless of having ignoring description and then a description in the assert... Maybe better to drop the description field in the assert json answer then, and having potentially a comment explaining quickly why, if somebody wants to give it a shot later?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #358: [REFACTORING] JMAP: All ids should be backed by IdConstraint
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #358:
URL: https://github.com/apache/james-project/pull/358#issuecomment-814538532
Force pushed to solve a rebase conflict...
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] mbaechler commented on a change in pull request #358: [REFACTORING] JMAP: All ids should be backed by IdConstraint
Posted by GitBox <gi...@apache.org>.
mbaechler commented on a change in pull request #358:
URL: https://github.com/apache/james-project/pull/358#discussion_r607213566
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxSet.scala
##########
@@ -46,7 +47,7 @@ case class MailboxSetRequest(accountId: AccountId,
onDestroyRemoveEmails: Option[RemoveEmailsOnDestroy]) extends WithAccountId
object MailboxSetRequest {
- type MailboxCreationId = String Refined NonEmpty
+ type MailboxCreationId = String Refined IdConstraint
Review comment:
As far as I understand Refined, `MailboxCreationId` type is equal to `UnparsedVacationResponseId` and thus defeat the purpose of having different types. You probably need newtype here. (see https://stackoverflow.com/questions/62579057/how-to-ensure-type-safety-with-scalas-refined-library-when-using-the-same-predi for example)
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala
##########
@@ -1059,7 +1059,7 @@ trait MailboxSetMethodContract {
| "notCreated": {
| "C42": {
| "type": "invalidArguments",
- | "description": "'/parentId' property in mailbox object is not valid: Predicate isEmpty() did not fail."
+ | "description": "'/parentId' property in mailbox object is not valid: Left predicate of ((!(0 < 1) && !(0 > 255)) && \\"\\".matches(\\"^[#a-zA-Z0-9-_]*$$\\")) failed: Predicate taking size() = 0 failed: Left predicate of (!(0 < 1) && !(0 > 255)) failed: Predicate (0 < 1) did not fail."
Review comment:
this looks like an over-precise check: it expects an error specific to Refined but should not
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa merged pull request #358: [REFACTORING] JMAP: All ids should be backed by IdConstraint
Posted by GitBox <gi...@apache.org>.
chibenwa merged pull request #358:
URL: https://github.com/apache/james-project/pull/358
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on a change in pull request #358: [REFACTORING] JMAP: All ids should be backed by IdConstraint
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #358:
URL: https://github.com/apache/james-project/pull/358#discussion_r606917208
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/VacationResponseGetMethodContract.scala
##########
@@ -482,14 +482,16 @@ trait VacationResponseGetMethodContract {
.asString
.stripMargin
- assertThatJson(response).isEqualTo(
+ assertThatJson(response)
+ .whenIgnoringPaths("methodResponses[0][1].description")
Review comment:
Why ignoring the description path here, and then why keeping it and changing the description in the assert below then?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #358: [REFACTORING] JMAP: All ids should be backed by IdConstraint
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #358:
URL: https://github.com/apache/james-project/pull/358#discussion_r607581136
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxSet.scala
##########
@@ -46,7 +47,7 @@ case class MailboxSetRequest(accountId: AccountId,
onDestroyRemoveEmails: Option[RemoveEmailsOnDestroy]) extends WithAccountId
object MailboxSetRequest {
- type MailboxCreationId = String Refined NonEmpty
+ type MailboxCreationId = String Refined IdConstraint
Review comment:
But thanks for pointing out this good practice that was of our radar...
2 hours fighting with the code base is still acceptable, no significant rework needed so I decided to give it a shot.
We might now have some "Id POJOs" that represents similar concepts and might be de-duplicated - but I will keep that for other PRs / refactorings.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #358: [REFACTORING] JMAP: All ids should be backed by IdConstraint
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #358:
URL: https://github.com/apache/james-project/pull/358#discussion_r607434478
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala
##########
@@ -1059,7 +1059,7 @@ trait MailboxSetMethodContract {
| "notCreated": {
| "C42": {
| "type": "invalidArguments",
- | "description": "'/parentId' property in mailbox object is not valid: Predicate isEmpty() did not fail."
+ | "description": "'/parentId' property in mailbox object is not valid: Left predicate of ((!(0 < 1) && !(0 > 255)) && \\"\\".matches(\\"^[#a-zA-Z0-9-_]*$$\\")) failed: Predicate taking size() = 0 failed: Left predicate of (!(0 < 1) && !(0 > 255)) failed: Predicate (0 < 1) did not fail."
Review comment:
This is over-precise because this error is generated at the JSON deserialization layer, and we likely have no to little context there.
Instead of relying on JSON refined implicit, then we would have to provide separate types deserialization wrapping our knowledge, I imagine.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #358: [REFACTORING] JMAP: All ids should be backed by IdConstraint
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #358:
URL: https://github.com/apache/james-project/pull/358#discussion_r607579554
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala
##########
@@ -1059,7 +1059,7 @@ trait MailboxSetMethodContract {
| "notCreated": {
| "C42": {
| "type": "invalidArguments",
- | "description": "'/parentId' property in mailbox object is not valid: Predicate isEmpty() did not fail."
+ | "description": "'/parentId' property in mailbox object is not valid: Left predicate of ((!(0 < 1) && !(0 > 255)) && \\"\\".matches(\\"^[#a-zA-Z0-9-_]*$$\\")) failed: Predicate taking size() = 0 failed: Left predicate of (!(0 < 1) && !(0 > 255)) failed: Predicate (0 < 1) did not fail."
Review comment:
Yes.
(I don't like the assertion neither)
But you are right that the description is cryptic ad lacks context. Hopefully it is now prefixed with a human readable message.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org