Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve vs TryResolve in subspace locations #145

Merged
merged 4 commits into from
Mar 20, 2025
Merged

Conversation

KrzysFR
Copy link
Member

@KrzysFR KrzysFR commented Mar 20, 2025

The current behavior of ISubspaceLocation.Resolve is to return null if the subspace does not exists, which causes a lot of inconsistencies in audited application code:

  • Force callers to add a lot of ! to hide the warnings
  • Completely ignore the case where it could return null, causing a lot of nullref when starting the application on a database that is not initialized (or if the path is invalid, typo, ...)
  • Add a check that is not really useful: the method called usually is in some business logic, and it cannot do anything about that (it is not its role to initialize the database!)

Looking at the code, only a handful of instances really need to handle the null case, mostly inside the binding itself, or in some test infrastructure, or database boostraper, most of the rest can simply be changed to a "required" semantic (ie: the method would throw with an appropriate error message).

This PR does the following:

  • change the existing Resolve() method signature to return ValueTask<TSubspace> (non nullable), and throw an InvalidOperationException if the location does not exist (or is invalid in some way)
  • add a new TryResolve() method with the old behavior (returns null if missing).
  • rename the weird Resolve(..., bool createIfMissing) into ResolveOrCreate(...).

What does it change in existing code?

  • Hopefully nothing. Any code that was simply using '!' or a null-check to make the warning "go away" will still work fine in the normal case, apart for maybe extra warning (since now the return value is non-null)
  • Only code that MUST know if the folder is missing have to be changed to call TryResolve().
  • Code that just wants the location to be auto-created should call ResolveOrCreate().

How to resolve a subspace in regular business logic code

Example before:

var subspace = (await tr.Resolve(location))!; // '!' used to mute the nullability warning
var key = subspace.Encode(/*.... */);

Example after:

var subspace = await tr.Resolve(location); // either throws or return a non-null subspace
var key = subspace.Encode(/*.... */);

How to detect the case when a subspace is missing

Example before:

var subspace = await tr.Resolve(location);
if (subspace == null)
{
    // handle missing case
}
else
{
    // handle existing case
}

Example after:

var subspace = await tr.TryResolve(location); // use "TryResolve" instead
if (subspace == null)
{
    // handle missing case
}
else
{
    // handle existing case
}

How to automatically create the subspace if it is missing

note: This is not a good practice! You should probably have some bootstraper code that will initialize everything properly, and let normal business logic fail if something is not right

Example before:

var subspace = await tr.Resolve(location, createIfMissing: true);
var key = subspace.Encode(/*.... */);

Example after:

var subspace = await tr.ResolveOrCreate(location);
var key = subspace.Encode(/*.... */);

KrzysFR added 4 commits March 20, 2025 17:37
- Change ISubspaceLocation.Resolve to throw if the folder does not exist ("required")
- Add ISubspaceLocation.TryResolve with the old behavior (return null if missing)
- TryResolve is only used by code that can do something about it (or provide a better error message)
- Resolve in all other cases (will throw by itself)
- Get rid of all the "!" and other nullability workaround that are now obsolete
@KrzysFR KrzysFR merged commit ecf138e into master Mar 20, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant