Return a wrapped fs.ErrNotExist if no spec directories are defined

[elezar]

See the discussion here: #308 (comment)

Here we return a wrapped fs.ErrNotExist error if there are no spec directories defined instead of a raw error that cannot be queried easily by the caller.

[elezar]

Thanks @klihub. While working on this, I was wondering whether we want to more strictly define (i.e. return an error from Configure) when no spec directories are defined? With that said, maybe this is a way for a user to instantiate an in-memory cache, but I would rather make that explicit rather than implicit.

[klihub]

Thanks @klihub. While working on this, I was wondering whether we want to more strictly define (i.e. return an error from Configure) when no spec directories are defined?

Yeah, at least my gut feeling is that it sounds like a reasonable idea.

Another option would be to update and document the cdi.WithSpecDirs option to ignore an empty director list and instead run with the defaults... since IFAICT using an explicit cdi.WithSpecDirs() is the only way how you can end up in this situation.

[elezar]

Another option would be to update and document the cdi.WithSpecDirs option to ignore an empty director list and instead run with the defaults... since IFAICT using an explicit cdi.WithSpecDirs() is the only way how you can end up in this situation.

I think this is a cleaner solution. Created #310 to do this.

[elezar]

rebased.

[fujitatomoya]

the fix looks good to me, but how about adding test case for this?

err := cache.RemoveSpec("something")
assert.True(t, errors.Is(err, fs.ErrNotExist))

[elezar]

I have added a basic test.

[elezar]

@klihub as also pointed out by the test, we do have a slight disconnect here. In the case where the spec dirs are empty, we return ErrNotExist, but in the case where os.Remove fails for the spec we are trying to remove due to ErrNotExist we return nil. Should we always return nil (or ErrNotExist) for consistency?

[klihub]

@elezar Yes, probably would make sense. Returning ErrNotExist as such would provide transparency for RemoveSpec() while returnin nil for ErrNotExist would provide idempotency. From the top of my head I don't know which would be better.

[elezar]

I think at this stage switching to returning ErrNotExists will end up breaking producers that use this code. I'm not sure if that's something we want to do. Let me add a comment to the code to this effect.

Also, as a follow-up, does this mean that we should rather return nil if there are no spec directories (especially given that this is now a case that we don't expect to ever occur)?

[klihub]

Also, as a follow-up, does this mean that we should rather return nil if there are no spec directories (especially given that this is now a case that we don't expect to ever occur)?

On the RemoveSpec() code path I think we could return nil if we have no spec directories if we return nil when there is no spec file to remove, as the former implies the latter.

I think we still might get into the situation when there are no spec dirs, because someone might nuke them while we are running.

[klihub]

I think at this stage switching to returning ErrNotExists will end up breaking producers that use this code. I'm not sure if that's something we want to do. Let me add a comment to the code to this effect.

Yes, I also think we should try to be extra conservative even when it feels like PITA. We're post 1.0, so we should not be making breaking changes. Even if it's not immediately obvious why and how someone could rely on existing functionality so that it would break if we start returning ErrNotExists, it's better not to risk especially if there is not much to gain by the change.

[klihub]

@elezar Can this still happen with #310 merged ?

[elezar]

It shouldn't be possible, no.

[elezar]

Does that mean we want to update the highestPrioritySpecDir implementation to not return an error?

[klihub]

I think it does not return an error. It returns a directory and its priority.