Refactor image vault utils #3846

Sploder12 · 2024-12-16T17:58:12Z

This PR refactors several functions related to image vault utilities. All util functions now have unit test coverage and can be mocked to ease testing for components that use the utilities. Some functions have been removed or moved to file_ops since they already exist or fit better there.

MULTI-1653

codecov · 2024-12-16T18:30:49Z

Codecov Report

Attention: Patch coverage is 85.93750% with 9 lines in your changes missing coverage. Please review.

Project coverage is 89.13%. Comparing base (cbcb5e7) to head (a57672d).
Report is 27 commits behind head on main.

Files with missing lines	Patch %	Lines
src/platform/backends/lxd/lxd_vm_image_vault.cpp	42.85%	4 Missing ⚠️
src/xz_decoder/xz_image_decoder.cpp	0.00%	3 Missing ⚠️
src/daemon/default_vm_image_vault.cpp	80.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3846      +/-   ##
==========================================
+ Coverage   89.02%   89.13%   +0.11%     
==========================================
  Files         255      255              
  Lines       14583    14604      +21     
==========================================
+ Hits        12982    13018      +36     
+ Misses       1601     1586      -15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tests/test_image_vault_utils.cpp

sharder996

Looks good @Sploder12

Thanks for tackling this. This has needed to be cleaned up for a while.

src/utils/file_ops.cpp

tests/test_file_ops.cpp

include/multipass/vm_image_vault.h

georgeliao

@Sploder12
Well done, I only have a few minor comments and questions.

georgeliao · 2025-01-20T10:51:54Z

include/multipass/vm_image_vault_utils.h

+    using DefaultDecoderT = XzImageDecoder;
+
+    virtual QString copy_to_dir(const QString& file, const QDir& output_dir) const;
+    [[nodiscard]] virtual QString compute_hash(QIODevice& device) const;


comput_hash function is used internally (except the unit test), do we have to expose that? Can it be a private function?

~~It could be made protected~~ nevermind

Is it the only role of it is being called as sub-function of the other compute_file_hash function? If so, I do not even see the point of unit testing that alone, because the unit test of compute_file_hash should cover that.

Anyway, I do not have a strong opinion on this. Feel free to go either way.

My reasoning for making it was to make compute_file_hash easier to unit test, since otherwise mocking would require a wrapper around the Qt hashing functions. Or it could use a real file for testing, but that tends to cause issues in CI so I wanted to avoid it.

ok I see now, basically QIODevice& device acted as a dependency injection point so you can pass down buffer as opposed to a file. I think we can compromise on this in this particular case. But In general, exposing private function for unit testing should not be the norm. Because of breaking encapsulation and increasing the test brittleness.

georgeliao · 2025-01-20T10:52:47Z

include/multipass/vm_image_vault_utils.h

+
+    virtual void verify_file_hash(const QString& file, const QString& hash) const;
+
+    virtual QString extract_file(const QString& file, const Decoder& decoder, bool delete_original = false) const;


Same question about the exposure of this non-template extract_file function.

Same response, in this case const Decoder& decoder (a function object) is used as an injection point.

georgeliao · 2025-01-20T11:08:14Z

tests/mock_image_decoder.h

+{
+
+class MockImageDecoder
+{


Nice standalone mock class only for template parameter use.

georgeliao · 2025-01-20T11:16:40Z

src/utils/vm_image_vault_utils.cpp

-    new_image_path.remove(".xz");
+QString mp::ImageVaultUtils::extract_file(const QString& file, const Decoder& decoder, bool delete_original) const
+{
+    const auto fs_new_path = MP_FILEOPS.remove_extension(file.toStdU16String());


Why exactly do we need the toStdU16String function as opposed to toStdString? I thought the toStdString checks the value type and return the corresponding string so it should be safe on windows, maybe I have missed something. Same question goes to fs_new_path.u8string() called later.

It is not safe to use a UTF-8 std::string on windows here. For example, see here. u8path solves this issue but is deprecated since c++20. Using the UTF-16 constructor of path, which is what this does, will always convert to the native encoding.

Another wondering is the QString::toStdU16String's behavior on linux, std::u16string is the input string to construct fs::path, will that be safe considering linux use UTF-8?

The path constructor overload for std::u16string says "conversion from UTF-16 to native filesystem encoding is used" so it should be safe. While the std::string constructor says "is assumed to be the native narrow encoding". This whole issue is probably why they added the std::u8string overload in C++20 which has the same spec as std::u16string, but with UTF-8

georgeliao · 2025-01-20T11:20:18Z

src/daemon/default_vm_image_vault.cpp

-                const auto image_filename = mp::vault::filename_for(image_url.path());
+
+                QFileInfo file_info{image_url.path()};
+                const auto image_filename = file_info.fileName();


Can we do one liner const auto image_filename = QFileInfo{image_url.path()}.fileName();? Which is cleaner. Same goes to other occurrences of this pattern in the PR.

I did so for where I could. The places with remove need an lvalue reference so a one liner won't work unless we change the API for the fileop.

src/utils/vm_image_vault_utils.cpp

tests/test_image_vault_utils.cpp

ricab reviewed Dec 17, 2024

View reviewed changes

tests/test_image_vault_utils.cpp Outdated Show resolved Hide resolved

jibel requested review from georgeliao and sharder996 January 16, 2025 15:47

sharder996 requested changes Jan 17, 2025

View reviewed changes

src/utils/file_ops.cpp Outdated Show resolved Hide resolved

tests/test_file_ops.cpp Show resolved Hide resolved

include/multipass/vm_image_vault.h Outdated Show resolved Hide resolved

Sploder12 added 6 commits January 17, 2025 09:17

Initial refactor of image vault utils

610b326

Add implementation of image vault utils

3150088

Move vault utils to singleton

66e5184

Polish image vault utils refactor

1bfca95

Add dummy var to appease nodiscard

5981042

Replace auto _ with std::ignore

4ec3486

Sploder12 force-pushed the refactor-image-vault-utils branch from f128abb to 4ec3486 Compare January 17, 2025 14:17

Sploder12 added 2 commits January 17, 2025 09:55

Address some review changes

d09fff0

Change remove_extension to use std path

f5c4598

sharder996 previously approved these changes Jan 17, 2025

View reviewed changes

georgeliao reviewed Jan 20, 2025

View reviewed changes

tests/test_image_vault_utils.cpp Outdated Show resolved Hide resolved

georgeliao mentioned this pull request Jan 21, 2025

Remove chmod and duplicated permissions functions #3782

Open

Address some review comments

a57672d

Sploder12 dismissed sharder996’s stale review via a57672d January 21, 2025 19:59

georgeliao approved these changes Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor image vault utils #3846

Refactor image vault utils #3846

Sploder12 commented Dec 16, 2024

codecov bot commented Dec 16, 2024 •

edited

Loading

sharder996 left a comment

georgeliao left a comment

georgeliao Jan 20, 2025

Sploder12 Jan 21, 2025 •

edited

Loading

georgeliao Jan 21, 2025 •

edited

Loading

georgeliao Jan 21, 2025 •

edited

Loading

Sploder12 Jan 21, 2025

georgeliao Jan 22, 2025

georgeliao Jan 20, 2025

Sploder12 Jan 22, 2025

georgeliao Jan 20, 2025

georgeliao Jan 20, 2025

Sploder12 Jan 21, 2025

georgeliao Jan 22, 2025

Sploder12 Jan 22, 2025

georgeliao Jan 20, 2025

Sploder12 Jan 21, 2025


		virtual void verify_file_hash(const QString& file, const QString& hash) const;

		virtual QString extract_file(const QString& file, const Decoder& decoder, bool delete_original = false) const;

Refactor image vault utils #3846

Are you sure you want to change the base?

Refactor image vault utils #3846

Conversation

Sploder12 commented Dec 16, 2024

codecov bot commented Dec 16, 2024 • edited Loading

Codecov Report

sharder996 left a comment

Choose a reason for hiding this comment

georgeliao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sploder12 Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

georgeliao Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

georgeliao Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 16, 2024 •

edited

Loading

Sploder12 Jan 21, 2025 •

edited

Loading

georgeliao Jan 21, 2025 •

edited

Loading

georgeliao Jan 21, 2025 •

edited

Loading