Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mongodb file download fix #333

Merged

Conversation

chf73
Copy link
Contributor

@chf73 chf73 commented May 4, 2023

We used the mongodb persistence in our try-out of the bays components and found the following issues when downloading files:

Fixes:

  • replace characters in filenames that may cause troubles by "_"
  • use a temporary directory (previously generated) to save files fetched from mongodb

@chf73 chf73 marked this pull request as ready for review May 4, 2023 10:19
@FrankSchnicke
Copy link
Contributor

Thank you very much for providing this PR! Would it be possible to sign the ECA?

@chf73
Copy link
Contributor Author

chf73 commented May 4, 2023

Thank you very much for providing this PR! Would it be possible to sign the ECA?

Done!

@FrankSchnicke
Copy link
Contributor

After some internal discussions, we realized that the issue is more complex than it seems. There are the following points that need to be considered:

  • Backwards compatibility: If we change the way how filenames are generated, a file stored in the old way is not findable anymore. Thus, the logic needs to be adapted in a way that it still works with the old names, e.g., checking for the new filename on retrieval and if it does not exist, checking for the old filename. Storing a file always should utilize the new filename.
  • Filename collision: By applying the regex, ids "id:" and "id/" would both be changed to "id_". Thus, if same idShorts are used, files may get overwritten. A possible solution for this would be to apply the regex as proposed by you and attaching the identifier-Hash to the resulting filename.

Would it be possible to update the PR the consider both issues?

@FrankSchnicke
Copy link
Contributor

Is this PR still active? If no, I will close it in a few days

@chf73
Copy link
Contributor Author

chf73 commented Jun 28, 2023

Is this PR still active? If no, I will close it in a few days

I am currently working on a solution that takes the named issues from your earlier comment into account, maybe I can provide a proposal this week.

chf73 and others added 6 commits June 29, 2023 12:30
…ownload_fix

# Conflicts:
#	basyx.components/basyx.components.docker/basyx.components.AASServer/src/main/java/org/eclipse/basyx/components/aas/mongodb/MongoDBSubmodelAPI.java
#	basyx.components/basyx.components.docker/basyx.components.AASServer/src/test/java/org/eclipse/basyx/regression/AASServer/mongodb/TestMongoDBSubmodelAPI.java
@FrankSchnicke
Copy link
Contributor

Thank you very much for updating the PR! Feel free to ping me when you're done with your implementations, I will then gladly review it again

@chf73
Copy link
Contributor Author

chf73 commented Jul 6, 2023

Thank you very much for updating the PR! Feel free to ping me when you're done with your implementations, I will then gladly review it again

Ping ;-) ... the last commit fixed the failing test, so I am finished now - thnx for reviewing again 👍

@FrankSchnicke
Copy link
Contributor

Looks good to me. Thank you very much for your contribution!

@FrankSchnicke FrankSchnicke merged commit 119d933 into eclipse-basyx:development Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants