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.

Sign up for GitHub

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

Jump to bottom

Add delete_files() to SharedRealm. #4701

Merged
merged 29 commits into from
May 27, 2021
Merged

Add delete_files() to SharedRealm. #4701

merged 29 commits into from
May 27, 2021

Conversation

DominicFrei
Copy link
Contributor

@DominicFrei DominicFrei commented May 19, 2021 •
edited
Loading

What, How & Why?

This PR adds the functionality to delete the files belonging to a SharedRealm (except for the .lock file) by offering delete_files().

These files are:

  • the Realm file itself
  • the .log file and it's legacy versions: .log_a and .log_b
  • the .note file
  • the .management folder

cc @nirinchev
This unlocks realm/realm-dotnet#386.

This PR could also subsitute these SDK implementations:

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)

Open questions:

  • I've seen the file ending used as strings in multiple locations. Is this preferred or could we extract them to one location that is used by them? And where would that preferably be?
  • Would we want to throw an exception instead of returning false when trying this on an open Realm or should this part actually be handled in the SDKs?

@DominicFrei DominicFrei self-assigned this May 19, 2021
@DominicFrei DominicFrei force-pushed the df/add-delete-realm-function branch from c1e8b4d to 0440c30 Compare May 19, 2021 16:12
@DominicFrei DominicFrei changed the title [WIP] Add delete_files() to SharedRealm. Add delete_files() to SharedRealm. May 19, 2021
@DominicFrei DominicFrei marked this pull request as ready for review May 19, 2021 17:13
@DominicFrei DominicFrei requested a review from finnschiermer May 19, 2021 17:13
ironage
ironage reviewed May 19, 2021
View reviewed changes
src/realm/object-store/shared_realm.cpp Outdated Show resolved Hide resolved
src/realm/object-store/shared_realm.cpp Outdated Show resolved Hide resolved
src/realm/object-store/shared_realm.hpp Outdated Show resolved Hide resolved
test/object-store/realm.cpp Outdated Show resolved Hide resolved
test/object-store/realm.cpp Show resolved Hide resolved
@DominicFrei DominicFrei force-pushed the df/add-delete-realm-function branch from 11346b0 to 7397771 Compare May 20, 2021 15:26
@DominicFrei DominicFrei requested a review from ironage May 20, 2021 15:28
@DominicFrei DominicFrei force-pushed the df/add-delete-realm-function branch 2 times, most recently from 7c3c784 to 1ebd738 Compare May 25, 2021 12:40
@DominicFrei DominicFrei force-pushed the df/add-delete-realm-function branch from f400a38 to c20b26c Compare May 25, 2021 14:22
@DominicFrei
Copy link
Contributor Author

I have added some changes with c20b26c to avoid even more strings and use the new get_core_files. The PR is ready for review again now, @ironage @finnschiermer . 👍

@DominicFrei DominicFrei requested a review from ironage May 26, 2021 12:27
@DominicFrei DominicFrei force-pushed the df/add-delete-realm-function branch from 6009d86 to 052a53f Compare May 26, 2021 12:30
@DominicFrei
Copy link
Contributor Author

With the rework to use a map the LangBindHelper_getCoreFiles does not pass anymore. After consideration (and checking again the new tests in test_db.cpp) this test should now be obsolete with the new behaviour and tests. If removed it in aa7b911.

@DominicFrei DominicFrei force-pushed the df/add-delete-realm-function branch from f60d964 to 25ab16b Compare May 26, 2021 17:57
ironage
ironage reviewed May 26, 2021
View reviewed changes
Copy link
Contributor

@ironage ironage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good, just some minor comments

src/realm/object-store/shared_realm.hpp Outdated Show resolved Hide resolved
test/test_lang_bind_helper.cpp Show resolved Hide resolved
@DominicFrei DominicFrei requested a review from ironage May 27, 2021 11:47
finnschiermer
finnschiermer suggested changes May 27, 2021
View reviewed changes
Copy link
Contributor

@finnschiermer finnschiermer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor thing, see comment.

src/realm/object-store/shared_realm.hpp Outdated Show resolved Hide resolved
@DominicFrei DominicFrei requested a review from finnschiermer May 27, 2021 13:54
finnschiermer
finnschiermer approved these changes May 27, 2021
View reviewed changes
Copy link
Contributor

@finnschiermer finnschiermer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ironage
ironage approved these changes May 27, 2021
View reviewed changes
Copy link
Contributor

@ironage ironage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@DominicFrei DominicFrei merged commit d394a87 into master May 27, 2021
@DominicFrei DominicFrei deleted the df/add-delete-realm-function branch May 27, 2021 15:50
@DominicFrei
Copy link
Contributor Author

Thanks guys!

This was referenced May 31, 2021
Merged
Closed
Closed
bmunkholm
bmunkholm reviewed Jun 4, 2021
View reviewed changes
CHANGELOG.md
@@ -1,7 +1,7 @@
# NEXT RELEASE

### Enhancements
* None.
* Added the functionality to delete files for a given SharedRealm, unlocking ([realm-dotnet#386](https://github.com/realm/realm-dotnet/issues/386)).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was missing a reference to this PR or an issue. It would also be good to mention what that function is called.
cc @ironage @finnschiermer @DominicFrei

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmunkholm why is the linked issue in dotnet insufficient?

Copy link
Contributor

@bmunkholm bmunkholm Jun 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ironage That doesn't give a reference to the code change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last comment in the GitHub issue links to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm- ok then. I was admittedly not reading that issue as it only said "unlocking" :-). I still think it doesn't hurt to mention the PR number to core here and which function has been added. Think about it from the perspective of another SDK team reading what new functionality has been added.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ironage ironage ironage approved these changes

@nirinchev nirinchev nirinchev left review comments

@bmunkholm bmunkholm bmunkholm left review comments

@finnschiermer finnschiermer finnschiermer approved these changes

Assignees

@DominicFrei DominicFrei

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DominicFrei @bmunkholm @nirinchev @ironage @finnschiermer