Adding validation for missing media items

[NillasKA]

Closes #20025

Description

Based on #20025
Validations were failing when you delete a media file from a created media item. The system wouldn't check if the media item was still there.

With this fix, I've added a check to the value's source path, to see if it's null or empty. If it is, we throw a null exception.

[AndyButland]

The check you are doing here looks reasonable @NillasKA, but I think it's not in the right place. This is a general validation service for all properties, and we don't know how all property editors might use or even allow an empty src field here.

Rather I think you should target this to a validation added to MediaEditingService.CreateAsync and .UpdateAsync that returns a status of PropertyValidationError in case of an empty src value provided.

Make sure to test on creates and updates, with and without an image file provided.

There may be an issue here on the front-end too - as, as far as I can see, this isn't a problem for image types, only audio (and maybe other) ones. So might be worth a bit of debugging to see if you can figure out why that is.

In any case though, it makes sense to validate this on the server.

[AndyButland]

Sorry @NillasKA - and you have permission to curse me - but I think we should actually move this logic once more. Where you have it isn't quite right as I don't think we should be checking here just for this JSON pattern.

Rather we should be doing this and the property editor's value editor. As that's where we generally have the logic for handling required fields. And really that's the issue here - this field is marked as required, but it's not being handled as such on the server. It would actually be perfectly legitimate for an implementor to modify this media type and make the file upload field optional. Unlikely in practice, but we should consistently support this.

So, if you look at BlockListEditorPropertyValueEditor - used for the block list - we have this line:

public override IValueRequiredValidator RequiredValidator => new BlockListValueRequiredValidator(JsonSerializer);

What this is doing is saying: for the block list, use the class BlockListValueRequiredValidator for the logic to determine what an empty provided value is.

For simple fields, like text strings, we have a default "required field validator" that simply checks if the value is empty.

But for more complicated property editors, like the block list, it's not enough to just look and see if the value is empty. It might not be, it might be a partly populated JSON structure, and we need to look at particular values within that to determine if we consider it "empty" and as such should fail the required check.

And the file upload field is similar.

So I think in FileUploadPropertyValueEditor you should, like we do for the block list, override the RequiredValidator method and provide an instance of a class that will do the check for empty. And that should use the method you have - looking at the src field to see if that has a value or not.

[AndyButland]

Looks very good now @NillasKA - I just pushed an update for a little tidy-up that you can look over.

Only thing remaining I think is that I saw we have BlockListValueRequiredValidatorTests, which defines some unit tests specifically for the block list required validator. I think you could create similar for FileUploadValueRequiredValidatorTests. Although this is tested via the integration tests, it would be quite easy to add these specific unit tests too, and means we have some tighter testing around this, can perhaps test more edge cases, etc.

[AndyButland]

Oh, and whilst we've been discussing moving code around and automated testing, it's worth me not forgetting there is a bug to fix here! I've tested what you've done and it works as expected. So when this last tests are added, I'll approve, and we can merge this and close the linked issue.

[AndyButland]

All looks good to me now @NillasKA, thanks.