You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "amichair (via GitHub)" <gi...@apache.org> on 2024/10/09 11:14:50 UTC
[PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
amichair opened a new pull request, #2444:
URL: https://github.com/apache/james-project/pull/2444
Changed the implementation to use a record per row design instead of a single row with concatenated addresses, as suggested in the bug report.
Along the way I also found that listSources was broken with such a multi-address value in the old design, and the fix is built-in in the new design. Added a regression test for it in the second commit.
The one thing that's missing is that I don't know if there is any procedure/mechanism/command for performing data/schema updates when an existing system is upgraded to a new version, is there such a thing?
--
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-unsubscribe@james.apache.org
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
Re: [PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on PR #2444:
URL: https://github.com/apache/james-project/pull/2444#issuecomment-2410209538
> Thanks for merging this!
With pleasure !
> I do wonder however, why all the nicely organized commits are squashed into one when merging? This makes it much harder for anyone else now or in the future to understand or review the code, with the various changes all bunched together...
Sorry maybe went a bit fast on this. However all commits should start with the JIRA ticket number, however only the first one was. I thought maybe little changes making it ok was ok to squash. Obviously I was wrong sorry for this.
--
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-unsubscribe@james.apache.org
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
Re: [PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
Posted by "amichair (via GitHub)" <gi...@apache.org>.
amichair commented on PR #2444:
URL: https://github.com/apache/james-project/pull/2444#issuecomment-2403340239
I added a draft of the migration instructions for now.
--
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-unsubscribe@james.apache.org
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
Re: [PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
Posted by "amichair (via GitHub)" <gi...@apache.org>.
amichair commented on PR #2444:
URL: https://github.com/apache/james-project/pull/2444#issuecomment-2405193229
I liked your idea, and it did take some work... I went for a webadmin export/import (seems more future-proof and intuitive to move jsons around instead of custom cli format parsing). Had to do some webadmin rearranging since mappings serialization wasn't fully implemented. Note that it's currently implemented as 'addMappings' rather than 'setMapping' (i.e. doesn't erase all existing mappings), I figured it's safer and more useful, though for the purposes of this migration it doesn't matter (the table is empty). Updated the instructions too. Didn't update the REST api docs with the new route, dunno when/how to do it, or maybe it can remain undocumented other than this migration ;-)
I added a few local tests, but didn't yet test the whole procedure for real - I'm assuming the table will get recreated when dropped but haven't tried it yet.
--
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-unsubscribe@james.apache.org
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
Re: [PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2444:
URL: https://github.com/apache/james-project/pull/2444#discussion_r1794212853
##########
upgrade-instructions.md:
##########
@@ -34,6 +34,36 @@ Change list:
- [Migrate RabbitMQ classic queues to version 2](#migrate-rabbitmq-classic-queues-to-version-2)
- [JAMES-3946 White list removals](#james-3946-white-list-removals)
- [JAMES-4052 Details in quota index](#james-4052-details-in-quota-index)
+ - [JAMES-1409 Change JPARecipientRewriteTable to store separate record per target address](#james-1409-change-jparecipientrewritetable-to-store-separate-record-per-target-address)
+
+### JAMES-1409 Change JPARecipientRewriteTable to store separate record per target address
+
+Date: 09/10/2024
+
+JIRA: https://issues.apache.org/jira/browse/JAMES-1409
+
+The JPARecipientRewriteTable was modified to store multiple mappings of a single source as separate database rows,
+each with a single target address, instead of a single row with a long semicolon-delimited multi-value target address.
+This solves both the limitation on the number of mappings (imposed by the column maximum length), and the broken query
+by target address.
+
+For JPA users, the database schema must be changed as following:
+
+- Change Primary key from (DOMAIN_NAME, USER_NAME) to (DOMAIN_NAME, TARGET_ADDRESS, USER_NAME) (unfortunately,
+ JPA auto-generates it in alphabetical order).
+- Add index USER_NAME_DOMAIN_NAME_TARGET_ADDRESS_INDEX (USER_NAME, DOMAIN_NAME, TARGET_ADDRESS).
+- Add index TARGET_ADDRESS_INDEX (TARGET_ADDRESS).
+- The columns themselves have not changed.
Review Comment:
Are the commands database specific or can we provide examples?
--
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-unsubscribe@james.apache.org
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
Re: [PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2444:
URL: https://github.com/apache/james-project/pull/2444#discussion_r1795932903
##########
server/data/data-library/src/test/java/org/apache/james/rrt/lib/RecipientRewriteTableContract.java:
##########
@@ -411,6 +411,19 @@ default void listSourcesShouldReturnWhenMultipleSourceMapping() throws Exception
assertThat(virtualUserTable().listSources(mapping)).contains(source, source2);
}
+ @Test
+ default void listSourcesShouldReturnWhenSourceHasMultipleMappings() throws Exception {
Review Comment:
Let's do also something for `getStoredMappings` ?
--
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-unsubscribe@james.apache.org
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
Re: [PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael merged PR #2444:
URL: https://github.com/apache/james-project/pull/2444
--
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-unsubscribe@james.apache.org
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
Re: [PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
Posted by "amichair (via GitHub)" <gi...@apache.org>.
amichair commented on code in PR #2444:
URL: https://github.com/apache/james-project/pull/2444#discussion_r1796120281
##########
server/data/data-library/src/test/java/org/apache/james/rrt/lib/RecipientRewriteTableContract.java:
##########
@@ -411,6 +411,19 @@ default void listSourcesShouldReturnWhenMultipleSourceMapping() throws Exception
assertThat(virtualUserTable().listSources(mapping)).contains(source, source2);
}
+ @Test
+ default void listSourcesShouldReturnWhenSourceHasMultipleMappings() throws Exception {
Review Comment:
I'm not sure what you mean. This was a bug I stumbled upon in the previous code while working on this, so I added a test just to make sure it no longer occurs in the new design. Everything else I assume should be covered by existing 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-unsubscribe@james.apache.org
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
Re: [PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
Posted by "amichair (via GitHub)" <gi...@apache.org>.
amichair commented on PR #2444:
URL: https://github.com/apache/james-project/pull/2444#issuecomment-2403271666
The changes are:
1. Primary key changed from (DOMAIN_NAME, USER_NAME) to (DOMAIN_NAME, TARGET_ADDRESS, USER_NAME) (unfortunately, it is auto-generated in alphabetical order)
2. Added index USER_NAME_DOMAIN_NAME_TARGET_ADDRESS_INDEX (USER_NAME, DOMAIN_NAME, TARGET_ADDRESS)
3. Added index TARGET_ADDRESS_INDEX (TARGET_ADDRESS)
4. Columns have not changed.
5. Rows that had multiple semicolon delimited values in their TARGET_ADDRESS field need to be split into individual rows each with a single target address value. Rows with a single value remain unchanged.
Afaik there is no standard SQL way of doing the last data migration step. I can write a method that does it, but is there any mechanism to invoke it? or just put it in a main method of the class? How have schema/data migrations been addressed in the past?
--
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-unsubscribe@james.apache.org
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
Re: [PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2444:
URL: https://github.com/apache/james-project/pull/2444#discussion_r1796527632
##########
server/data/data-library/src/test/java/org/apache/james/rrt/lib/RecipientRewriteTableContract.java:
##########
@@ -411,6 +411,19 @@ default void listSourcesShouldReturnWhenMultipleSourceMapping() throws Exception
assertThat(virtualUserTable().listSources(mapping)).contains(source, source2);
}
+ @Test
+ default void listSourcesShouldReturnWhenSourceHasMultipleMappings() throws Exception {
Review Comment:
With the export import procedure via webadmin it may not be relevant.
Let's keep as is.
--
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-unsubscribe@james.apache.org
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
Re: [PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2444:
URL: https://github.com/apache/james-project/pull/2444#discussion_r1794217329
##########
upgrade-instructions.md:
##########
@@ -34,6 +34,36 @@ Change list:
- [Migrate RabbitMQ classic queues to version 2](#migrate-rabbitmq-classic-queues-to-version-2)
- [JAMES-3946 White list removals](#james-3946-white-list-removals)
- [JAMES-4052 Details in quota index](#james-4052-details-in-quota-index)
+ - [JAMES-1409 Change JPARecipientRewriteTable to store separate record per target address](#james-1409-change-jparecipientrewritetable-to-store-separate-record-per-target-address)
+
+### JAMES-1409 Change JPARecipientRewriteTable to store separate record per target address
+
+Date: 09/10/2024
+
+JIRA: https://issues.apache.org/jira/browse/JAMES-1409
+
+The JPARecipientRewriteTable was modified to store multiple mappings of a single source as separate database rows,
+each with a single target address, instead of a single row with a long semicolon-delimited multi-value target address.
+This solves both the limitation on the number of mappings (imposed by the column maximum length), and the broken query
+by target address.
+
+For JPA users, the database schema must be changed as following:
+
+- Change Primary key from (DOMAIN_NAME, USER_NAME) to (DOMAIN_NAME, TARGET_ADDRESS, USER_NAME) (unfortunately,
+ JPA auto-generates it in alphabetical order).
+- Add index USER_NAME_DOMAIN_NAME_TARGET_ADDRESS_INDEX (USER_NAME, DOMAIN_NAME, TARGET_ADDRESS).
+- Add index TARGET_ADDRESS_INDEX (TARGET_ADDRESS).
+- The columns themselves have not changed.
+
+In addition, the existing data must be migrated to the new structure:
+
+- Rows with a single address in the TARGET_ADDRESS column are unchanged.
+- Rows that have multiple semicolon-delimited addresses in their TARGET_ADDRESS column need to be split into
+ individual rows, each with a single target address value.
+ Unfortunately, there is no standard way to do this in SQL. In some databases it may be possible to build
+ a recursive query that can do this. Otherwise, it can be done manually using database management tools,
+ or via a short external script, or via exporting the data, manual editing it, and re-importing the migrated
+ data.
Review Comment:
Or we could:
- Ask the user to list all mappings through the CLI prior to the upgrade
- Change the table name: a new table will be used, empty
- Provide a new CLI command to import the mappings from the previously saved input
That way user do not looses data.
WDYT?
--
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-unsubscribe@james.apache.org
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
Re: [PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2444:
URL: https://github.com/apache/james-project/pull/2444#discussion_r1794217329
##########
upgrade-instructions.md:
##########
@@ -34,6 +34,36 @@ Change list:
- [Migrate RabbitMQ classic queues to version 2](#migrate-rabbitmq-classic-queues-to-version-2)
- [JAMES-3946 White list removals](#james-3946-white-list-removals)
- [JAMES-4052 Details in quota index](#james-4052-details-in-quota-index)
+ - [JAMES-1409 Change JPARecipientRewriteTable to store separate record per target address](#james-1409-change-jparecipientrewritetable-to-store-separate-record-per-target-address)
+
+### JAMES-1409 Change JPARecipientRewriteTable to store separate record per target address
+
+Date: 09/10/2024
+
+JIRA: https://issues.apache.org/jira/browse/JAMES-1409
+
+The JPARecipientRewriteTable was modified to store multiple mappings of a single source as separate database rows,
+each with a single target address, instead of a single row with a long semicolon-delimited multi-value target address.
+This solves both the limitation on the number of mappings (imposed by the column maximum length), and the broken query
+by target address.
+
+For JPA users, the database schema must be changed as following:
+
+- Change Primary key from (DOMAIN_NAME, USER_NAME) to (DOMAIN_NAME, TARGET_ADDRESS, USER_NAME) (unfortunately,
+ JPA auto-generates it in alphabetical order).
+- Add index USER_NAME_DOMAIN_NAME_TARGET_ADDRESS_INDEX (USER_NAME, DOMAIN_NAME, TARGET_ADDRESS).
+- Add index TARGET_ADDRESS_INDEX (TARGET_ADDRESS).
+- The columns themselves have not changed.
+
+In addition, the existing data must be migrated to the new structure:
+
+- Rows with a single address in the TARGET_ADDRESS column are unchanged.
+- Rows that have multiple semicolon-delimited addresses in their TARGET_ADDRESS column need to be split into
+ individual rows, each with a single target address value.
+ Unfortunately, there is no standard way to do this in SQL. In some databases it may be possible to build
+ a recursive query that can do this. Otherwise, it can be done manually using database management tools,
+ or via a short external script, or via exporting the data, manual editing it, and re-importing the migrated
+ data.
Review Comment:
What happens if I send an email to a recipient that has several rrt rules stored in the old format with a James expecting the new format?
*Idea 1*
Or we could:
- Ask the user to list all mappings through the CLI prior to the upgrade
- Change the table name: a new table will be used, empty
- Provide a new CLI command to import the mappings from the previously saved input
That way user do not looses data.
WDYT?
Cons: likely quite a bunch of code: I'd understand that people get lazy...
*Idea 2*:
If one of the mapping is actually an older value we could stil access it and "flatMap" it transparently to the user, thus addressing (partially) retro-compatibility, at least for reads...
--
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-unsubscribe@james.apache.org
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
Re: [PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #2444:
URL: https://github.com/apache/james-project/pull/2444#issuecomment-2405812962
> I went for a webadmin export/import (seems more future-proof and intuitive to move jsons around instead of custom cli format parsing).
+1
--
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-unsubscribe@james.apache.org
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
Re: [PR] James 1409 - Change JPARecipientRewriteTable to store separate record per target address [james-project]
Posted by "amichair (via GitHub)" <gi...@apache.org>.
amichair commented on PR #2444:
URL: https://github.com/apache/james-project/pull/2444#issuecomment-2410161298
Thanks for merging this!
I do wonder however, why all the nicely organized commits are squashed into one when merging? This makes it much harder for anyone else now or in the future to understand or review the code, with the various changes all bunched together...
--
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-unsubscribe@james.apache.org
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