Map Forbidden File IO exceptions to 403

[andrew4699]

Description

Makes Forbidden exceptions during File IO return a 403.

1 line change turned into 1000 lines of test changes...

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

• Integration test

Checklist:

• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings

[flyrain]

Optional is designed for method return value, but not for a class field or method parameter. It adds overhead and confusing without much benefits. This double-wrapping(Optional<Supplier<RuntimeException>>) raises a bit more questions:

1. Is the Supplier optional, or is the result of the supplier function (the RuntimeException) optional?
2. Why does the exception need to be wrapped in a supplier rather than passed directly?

Could we use a simpler form like this:

RuntimeException newInputFileException;

Then we can throw it by checking if it's null:

if(newInputFileException != null) throw newInputFileException;

Or if you really want to use optional, we can wrap it within a getter like this, and invoke it from callers:

Optional<RuntimeException> getNewInputFileException(){
   return Optional.ofNullable(newInputFileException);
} 

I think using null is perfectly fine here. This simplifies all callers in the chain.

[eric-maynard]

It adds overhead and confusing without much benefits. This double-wrapping(Optional<Supplier>) raises a bit more questions:

What exactly is unclear about Optional<Supplier<...>>? It seems pretty obvious from the name that the caller may optionally provide a supplier.

Is the Supplier optional, or is the result of the supplier function (the RuntimeException) optional?

In fact with Optional<Supplier<..>> the caller has only one choice and it's checked at compile time. The supplier itself is optional.

On the other hand, if we rely on null, they can either pass a null supplier or a supplier that yields null. Which is it?

Why does the exception need to be wrapped in a supplier rather than passed directly?

In this case I think it's fine to remove the supplier, but relying on the exception itself being null isn't that much more clear.

[flyrain]

Using Optional here adds cognitive burden for the reader, especially since Optional is typically used to facilitate fluent method chaining. This additional complexity can be confusing, particularly for someone quickly scanning the code. Plus, it doesn't add any benefit here.

On the other hand, if we rely on null, they can either pass a null supplier or a supplier that yields null. Which is it?

() -> null is not a null supplier, right?

[andrew4699]

Since it's a test, the exception technically doesn't have to be wrapped in a Supplier, but more generally exceptions generate their stack trace at construction time so using a Supplier makes the stack trace accurate.

Regarding Optional vs. null vs. () -> null, I don't have a strong preference. They all seem equally (un)confusing and the reader can look at the comments to figure out the meaning.

[flyrain]

Supplier makes sense if we want accurate stack trace.

I don't think it's necessary to use Optional though. Optional<Supplier<RuntimeException>> can also accept () -> null, as the supplier is not null. BTW, we may check if s.get() is null here, as it may throw a null here.

   newInputFileExceptionSupplier.ifPresent(
        s -> {
          throw s.get();
        });

[eric-maynard]

Yes, Optional<A<B<..>> can always accept the A being null, or the B being null, or B.c.x.y.z being null. But the point of optional types is to make it obvious to the caller exactly at what level the optionality lies. Without it, they are left to read the code to guess what exact reference needs to be null (if any) for things to work.

Instead of pursuing 110% null-safety at every level of the object hierarchy across all APIs, it's better to be explicit and use Optional to clearly demarcate what objects are "optional". If the caller passes in a null for A.B.c even though A was optional, this is pretty clearly a bug and not an appropriate way to disable behavior. That's less clear if A is not optional and we are using nulls as flow control.

Having said all of this, it's a test class. Because of that I think it's fair to expect folks using this to read the code, and so all of this doesn't matter very much. I am okay merging this either way.

[flyrain]

Makes sense to pursue null-safety. The Optional isn't designed for that though. It works in a clumsy way to achieve that, you will have to add Optional to the signatures of all callers, and serialization won't work IIUC. It doesn't quite matter here anyway. I'm fine with either one.