[PR] UNOMI-986: Write a one-shot import CSV to the directory of its tenant [unomi]

["jsinovassin (via GitHub)" <gi...@apache.org>]

jsinovassin opened a new pull request, #867:
URL: https://github.com/apache/unomi/pull/867

   Fixes UNOMI-986, at https://issues.apache.org/jira/browse/UNOMI-986
   
   ### What this changes
   
   `ImportConfigurationServiceEndPoint.processOneshotImportConfigurationCSV` writes the uploaded CSV to a directory named after the current tenant, in place of the root of the upload directory.
   
   ### Evidence
   
   The endpoint and the Camel route that consumes the file disagreed on the path.
   
   The endpoint wrote the file to `<uploadDir>/<importConfigId>.csv`:
   
   ```java
   java.nio.file.Path path = Paths.get(configSharingService.getProperty(RouterConstants.IMPORT_ONESHOT_UPLOAD_DIR) + importConfigId + ".csv");
   ```
   
   `ImportConfigByFileNameProcessor.process` reads the tenant from the parent directory of the file:
   
   ```java
   String tenantId = extractTenantId(filePath);
   if (tenantId == null || !isValidTenantId(tenantId) || !isValidTenant(tenantId)) {
       LOGGER.warn("Invalid or missing tenant ID in path: {}", filePath);
       exchange.setProperty(Exchange.ROUTE_STOP, Boolean.TRUE);
       return;
   }
   ```
   
   `extractTenantId` returns the name of the parent directory. For a file at the root of the upload directory that name is `unomi_oneshot_import_configs`, which is not a tenant. Every one-shot import uploaded through the endpoint was therefore dropped:
   
   ```
   WARN ImportConfigByFileNameProcessor | Invalid or missing tenant ID in path:
     /opt/unomi/data/tmp/unomi_oneshot_import_configs/19d061b5-b03d-41ae-beeb-8d75c4f1bf22.csv
   ```
   
   The caller saw no failure. The upload answered 200, and the route moved the file to `.camel` unprocessed.
   
   ### Mechanism
   
   The route already reads sub-directories, because `ProfileImportOneShotRouteBuilder` declares `recursive=true`:
   
   ```java
   from("file://" + uploadDir + "?recursive=true&moveFailed=.error&include=.*.csv")
   ```
   
   The per-tenant directory is what the route expected all along, so the endpoint is the side that changes. It resolves the current tenant through `ExecutionContextManager`, creates the directory, and writes the file into it.
   
   ### Effect
   
   Measured on Apache Unomi 3.1.0-SNAPSHOT.
   
   Before, the upload answered 200 and no profile was imported. Now:
   
   ```
   POST /cxs/importConfiguration/oneshot   -> 200
   <uploadDir>/default/<importConfigId>.csv
   ```
   
   The route reads the file and the profiles of the CSV are imported. An end to end test that drives the import through the user interface went from failing to passing.
   
   ### Notes for the reviewer
   
   `router-rest` carries no test source directory, so this pull request adds no unit test. The verification above is end to end.
   
   `ExportConfigurationServiceEndPoint` carries a `/oneshot` path as well. This pull request does not cover the export side, which is worth checking for the same disagreement.
   


-- 
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: dev-unsubscribe@unomi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] UNOMI-986: Write a one-shot import CSV to the directory of its tenant [unomi]

["sergehuber (via GitHub)" <gi...@apache.org>]

sergehuber commented on PR #867:
URL: https://github.com/apache/unomi/pull/867#issuecomment-5732311720

   Thank you, Jonathan — this is a clean diagnosis of a bug that is easy to miss, and I am grateful you followed the Camel route all the way through. One-shot import taking the tenant from the parent directory, while the endpoint wrote at the upload-dir root, fully explains why those files were dropped. Writing `{uploadDir}/{tenantId}/{importConfigId}.csv` matches that contract nicely.
   
   Two small suggestions, only if they are easy for you. Neither should be a lot of work, and I am happy to help:
   
   - A null-check on `executionContextManager.getCurrentContext()` would turn a missing context into a 4xx instead of an NPE. Something like returning 400/409 when `getCurrentContext()` is null would be plenty.
   - This same file is also touched in #853 (`@RequiresRole` on the endpoint). Rebasing onto #853 (or waiting until it lands) would save a merge conflict. No rush if you would rather wait.
   
   I would not block on a unit test — `router-rest` has no real harness for this path today. A one-liner asserting the destination path would be a kindness to the next person, nothing more.
   
   Thank you again for this. I would be glad to merge it once it sits cleanly next to #853.


-- 
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: dev-unsubscribe@unomi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] UNOMI-986: Write a one-shot import CSV to the directory of its tenant [unomi]

["sergehuber (via GitHub)" <gi...@apache.org>]

sergehuber merged PR #867:
URL: https://github.com/apache/unomi/pull/867


-- 
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: dev-unsubscribe@unomi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org