[#2181] Redesign FileAPI to use original file name #2300
Conversation
github-actions
bot
added
java
There was a problem hiding this comment.
Thank you very much for this PR and the detailed documentation @muyangye. I really appreciate!
I've left some minor remarks for improvements.
@dominikriemer Can you give the UI changes a review?
streampipes-rest/src/main/java/org/apache/streampipes/rest/impl/PipelineElementFile.java
Outdated
Show resolved
Hide resolved
streampipes-rest/src/main/java/org/apache/streampipes/rest/impl/PipelineElementFile.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public void executeMigration() { |
There was a problem hiding this comment.
Would be great if you could add some documentation here about why this migration is necessary. So that we easily can relate this migration to it's purpose later on as well.
There was a problem hiding this comment.
That's a good suggestion! I'll add some documentation.
.../java/org/apache/streampipes/service/core/migrations/v095/DuplicateFilesRenameMigration.java
Outdated
Show resolved
Hide resolved
ui/src/app/files/dialog/file-rename/file-rename-dialog.component.html
Outdated
Show resolved
Hide resolved
…service/core/migrations/v095/DuplicateFilesRenameMigration.java Co-authored-by: Tim <50115603+bossenti@users.noreply.github.com>
…nt.html Co-authored-by: Tim <50115603+bossenti@users.noreply.github.com>
…l/PipelineElementFile.java Co-authored-by: Tim <50115603+bossenti@users.noreply.github.com>
…l/PipelineElementFile.java Co-authored-by: Tim <50115603+bossenti@users.noreply.github.com>
There was a problem hiding this comment.
Hi @muyangye,
thanks a lot for your PR. I think it's very good.
I have created another branch myself in which I have written a unit test for the DuplicateFilesRenameMigrationTest.executeMigration() method. I also refactored it a bit. What do you think? Do you think this iteration is an improvement? Link to branch
Hi @tenthe, thanks a lot for the refactoring! I think those code are much more self-explanatory! Thanks for the unit tests too. I didn’t write unit tests because this migration is a temporary solution. I’ll come up with unit tests for my next PR. Btw I still haven’t added a comment that Tim suggested and I will do that once I land (checking in for a flight right now) :) |
|
@bossenti @tenthe Sorry for the late reply. I am currently in China and I was trying to solve Github being blocked for the past 2 days and found no luck: |
|
Closing this PR as updated by #2356. |
* Serialize non-primitive types and store in Influx * extract RawFieldSerializer * rename test * delete old * temp * redesign file api * fix * Update streampipes-service-core/src/main/java/org/apache/streampipes/service/core/migrations/v095/DuplicateFilesRenameMigration.java Co-authored-by: Tim <50115603+bossenti@users.noreply.github.com> * Update ui/src/app/files/dialog/file-rename/file-rename-dialog.component.html Co-authored-by: Tim <50115603+bossenti@users.noreply.github.com> * Update streampipes-rest/src/main/java/org/apache/streampipes/rest/impl/PipelineElementFile.java Co-authored-by: Tim <50115603+bossenti@users.noreply.github.com> * Update streampipes-rest/src/main/java/org/apache/streampipes/rest/impl/PipelineElementFile.java Co-authored-by: Tim <50115603+bossenti@users.noreply.github.com> * refactor(#2181): Refactor DuplicateFilesRenameMigration * Add comment for migration * fix(#2356): Fix linting error * fix(#2356): Fix checkstyle proble --------- Co-authored-by: muyangye <muyangye@usc.edu> Co-authored-by: Tim <50115603+bossenti@users.noreply.github.com>
Purpose
Try to fix #2181
Remarks
PR introduces (a) breaking change(s): no
PR introduces (a) deprecation(s): no
After this change, if the users are trying to upload a file that already exists (same original file name ignoring case):
They can upload just like before after rename:
Similarly, when users can upload multiple files and some of them already exist:
The dialog will keep appearing if even after rename, those files still exist:
As you may noticed, there are already some files with same name in the background. I created those files before this change to test migration. After the migration, they shall be renamed:
I have also created adapters using those files and verified they still work (as well as pipelines using those adapters):
I have not implemented E2E tests for this new feature because the discussion of #2181 is ongoing. If everyone is happy with this solution. I'll go ahead create an issue to implement the E2E tests.
I know the code is a little bit long. So TLDR, I have kept
internalFilenameandoriginalFilenameto avoid damages to other processes using current files. However, I added an endpoint/allFilenamesinPipelineElementFilethat fetches all currentoriginalFilenames so that in the frontend I can check if some files users uploaded already exist and if so, let users rename those files. In addition, since before this change, there are probably some files with the same name, I have implemented aDuplicateFilesRenameMigrationto rename those files'originalFilename.