Add s3 disk validation step #22

lehecht · 2025-02-06T09:24:25Z

Update Tests

Closes #4.

lehecht · 2025-02-06T09:36:50Z

@mzur
Could you give some feedback on my validation method?
I had to use Storage::build() instead of Storage::disk()because disk() makes the update method unnecessarily complex. With disk(), I would need to use DB::transaction() to first save the updated fields, then validating them and maybe rolling everything back if there is an error. Relying solely on the request wasn’t an option because disk() uses the disk from the database, which hadn’t been updated, yet.

mzur · 2025-02-06T10:39:37Z

I think using a database transaction for this would be quite elegant! Then you can use disk() too, which should work with other types than S3 in the future.

I would advise against using allFiles() to check the disk. If there are a lot of files, the memory limit could be reached. I recently had a case where a user had 100k files and it crashed. I implemented a more efficient check for content that you could use here, too. I would not throw en error if the disk is empty, though. Maybe users want to prepare the disk before they upload data to it. Use the test only to check for configuration errors.

mzur · 2025-02-06T12:10:01Z

It just came to my mind: You can probably use the VolumeUrl validation rule directly here. No need to duplicate code (unless it prevents you from showing a helpful error message).

lehecht · 2025-02-07T07:59:03Z

@mzur
The code you mentioned is located in passesDiskUrl(), but when I use VolumeUrl with the disk endpoint, it uses passesRemoteUrl().
After duplicating the code, I no longer get an error if the endpoint URL is incorrect; instead, it just returns an empty list.

lehecht · 2025-02-07T08:22:37Z

I found that an exception is only thrown if the host url is incorrect. If the bucket name is missing or wrong, it just returns an empty list.

mzur · 2025-02-10T13:39:48Z

When I place the listContents() before the try/catch in your validateS3Config() I get errors as expected. The only thing that is not validated properly, it seems, is when I enter a wrong bucket name in the endpoint URL. This runs into the execution timeout. Please push your current code so I can have a look.

Make code more elegant by using Storage::disk and DB::transaction.

lehecht · 2025-02-11T08:26:53Z

@mzur
My problem is resolved. I'm not sure what I did differently this time, but incorrect bucket names trigger errors now.
I've pushed my code, so you can review it again. Everything should be working, but I need some feedback on my error messages. Thank you for your help!

I'll add tests later after my method doesn't need to be changed any more. So I'm not finished, yet.

src/Http/Controllers/Api/UserDiskController.php

src/resources/views/update.blade.php

src/resources/views/store/s3.blade.php

src/resources/views/update/s3.blade.php

lehecht · 2025-02-24T07:35:12Z

The tests are failing because they still use docker-compose instead of docker compose. Can I update the command in this PR or should I open a new one?

mzur · 2025-02-24T12:21:03Z

You can update the command in this PR.

mzur

Some comments left. Please be a little more diligent next time.

mzur · 2025-03-06T11:52:44Z

src/Http/Controllers/Api/UserDiskController.php

 use Biigle\Modules\UserDisks\Http\Requests\StoreUserDisk;
 use Biigle\Modules\UserDisks\Http\Requests\ExtendUserDisk;
 use Biigle\Modules\UserDisks\Http\Requests\UpdateUserDisk;
+use Biigle\Modules\UserDisks\Exceptions\DiskValidationException;


Where is this thrown? I only find ValidationException.

mzur · 2025-03-06T11:55:56Z

src/Http/Controllers/Api/UserDiskController.php

        $options = $disk->options;
        $endpoint = $options['endpoint'];
        $bucket = $options['bucket'];

        // Check if endpoint contains bucket name
        if (!preg_match("/(\b\/" . $bucket . "\.|\b" . $bucket . "\b)/", $endpoint)) {
-            $errors['endpoint'] = 'Endpoint url requires bucket name';
-            return $errors;
+            throw ValidationException::withMessages(['endpoint' => 'Endpoint url must contain bucket name. Please check if name is present and spelled correctly.']);


Please check if proper English is used 😜 Seriously, these kinds of things must be user-friendly.

Suggested change

throw ValidationException::withMessages(['endpoint' => 'Endpoint url must contain bucket name. Please check if name is present and spelled correctly.']);

throw ValidationException::withMessages(['endpoint' => 'The endpoint URL must contain the bucket name. Please check if the name is present and spelled correctly.']);

mzur · 2025-03-06T11:58:50Z

src/Http/Controllers/Api/UserDiskController.php

-            $files = $disk->getAdapter()->listContents('/', false);
-            // Need to access an element to check if endpoint url is valid
-            $files->current();
+            $this->canAccessDisk($disk);


Why put this in an extra method if it is only used once?

Ah it's for testing. Please add a docblock to the method (which should be there anyway) and explain this in the comment.

The method name may be a little misleading because the method returns void and not bool. Maybe call it validateDiskAccess?

mzur · 2025-03-06T11:59:15Z

src/Http/Controllers/Api/UserDiskController.php

@@ -171,42 +172,41 @@ public function destroy($id)
    protected function validateS3Config(UserDisk $disk)


Please add a docblock to this method.

mzur · 2025-03-06T12:56:07Z

src/resources/views/store.blade.php

+                    <div class="col-xs-12" @error('error') has-error @enderror>
+                        @error('error')
+                            <div class="panel panel-danger">
+                                <div class="panel-body text-danger">
+                                    {{$message}}
+                                </div>
+                            </div>
+                        @enderror
+                    </div>


Please still apply the same change than in update.blade.php. And please take a little more care next time when you work through my review comments, you are our "senior" dev after all 😉

mzur · 2025-03-06T13:09:30Z

src/Http/Controllers/Api/UserDiskController.php

+        } catch (DiskValidationException $e) {
+            return $this->fuzzyRedirect()
+                ->withErrors($e->getValidationError())
+                ->withInput();
+        }


Since the DiskValidationException doesn't seem to exist, the try/catch can probably be removed. Please be a little more careful about something like this. Mistakes like these shouldn't exist any more in a review-ready PR.

mzur · 2025-03-06T13:15:13Z

tests/Http/Controllers/Api/UserDiskControllerTest.php

        $this->postJson("/api/v1/user-disks", [
                'name' => 'my disk',
                'type' => 's3',
            ])
            ->assertStatus(422);

+        $this->mockS3->shouldReceive('canAccessDisk')->once()->andReturn([]);


Why return an array when the original method returns void?

mzur · 2025-03-06T13:19:13Z

tests/Http/Controllers/Api/UserDiskControllerTest.php

I couldn't find any test where canAccessDisk() actually throws an exception. Didn't you test these (most important) cases at all?

lehecht added 3 commits February 6, 2025 09:04

Add s3 config validation

0d4851a

Show error message if s3 config is invalid

d1c3dc6

Move help texts to only highlight errors

7857719

Rewrite code

705a721

Make code more elegant by using Storage::disk and DB::transaction.

mzur requested changes Feb 11, 2025

View reviewed changes

lehecht added 9 commits February 12, 2025 10:19

Change error messages

7bf5a70

Throw exception if s3 disk config is invalid

8ece2ff

Change code structure to increase code readability

8a0f2f3

Throw validation exeptions that uses array messages

846cf59

Fix transaction return statement

2deebc0

Refactor code to enable testing

4e1bc98

Add and update tests

a1e01e6

Refactor code

9b9bea2

Fix failing tests

9c3afcf

lehecht force-pushed the add-s3-disk-validation-step branch 2 times, most recently from 15e96e7 to fb8a21f Compare February 24, 2025 07:27

Minor changes

16ee9be

lehecht force-pushed the add-s3-disk-validation-step branch from fb8a21f to 16ee9be Compare February 24, 2025 07:28

lehecht requested a review from mzur February 24, 2025 07:31

Update test.yml

2f4c758

lehecht marked this pull request as ready for review February 25, 2025 08:09

mzur reviewed Mar 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add s3 disk validation step #22

Add s3 disk validation step #22

lehecht commented Feb 6, 2025 •

edited

Loading

lehecht commented Feb 6, 2025 •

edited

Loading

mzur commented Feb 6, 2025

mzur commented Feb 6, 2025 •

edited

Loading

lehecht commented Feb 7, 2025

lehecht commented Feb 7, 2025

mzur commented Feb 10, 2025

lehecht commented Feb 11, 2025 •

edited

Loading

lehecht commented Feb 24, 2025

mzur commented Feb 24, 2025

mzur left a comment

mzur Mar 6, 2025

mzur Mar 6, 2025

mzur Mar 6, 2025

mzur Mar 6, 2025

mzur Mar 6, 2025

mzur Mar 6, 2025

mzur Mar 6, 2025

mzur Mar 6, 2025

mzur Mar 6, 2025

mzur Mar 6, 2025

	throw ValidationException::withMessages(['endpoint' => 'Endpoint url must contain bucket name. Please check if name is present and spelled correctly.']);
	throw ValidationException::withMessages(['endpoint' => 'The endpoint URL must contain the bucket name. Please check if the name is present and spelled correctly.']);

		@@ -171,42 +172,41 @@ public function destroy($id)
		protected function validateS3Config(UserDisk $disk)

Add s3 disk validation step #22

Are you sure you want to change the base?

Add s3 disk validation step #22

Conversation

lehecht commented Feb 6, 2025 • edited Loading

lehecht commented Feb 6, 2025 • edited Loading

mzur commented Feb 6, 2025

mzur commented Feb 6, 2025 • edited Loading

lehecht commented Feb 7, 2025

lehecht commented Feb 7, 2025

mzur commented Feb 10, 2025

lehecht commented Feb 11, 2025 • edited Loading

lehecht commented Feb 24, 2025

mzur commented Feb 24, 2025

mzur left a 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

lehecht commented Feb 6, 2025 •

edited

Loading

lehecht commented Feb 6, 2025 •

edited

Loading

mzur commented Feb 6, 2025 •

edited

Loading

lehecht commented Feb 11, 2025 •

edited

Loading