`null` values in the `Map.toDomain(c)` return.

[ydennisy]

hi @stemmlerjs - first of all thanks for some great blog posts & this repo.

There are multiple places where we would get a list from the repo and then do:

return resultFromRepo.map(SomeMap.toDomain);

We are therefore returning (Entity | null)[] is this expected? At what layer in our application shall we filter / clean this list to return only valid values?

[ydennisy]

I wanted to elaborate on this question a little as it is still causing me some trouble in the design...

So right now this project maps object from the database into domain objects directly in the repository, this seems a nice place to perform this action, however the issue is that a call to the repository can return null, as all the EntityMap classes will return null if creation fails.

This seems a little problematic, for example this use case, which I have stripped down would not type check as the repository could return null:

export class DownvotePost implements UseCase<DownvotePostDTO, Promise<DownvotePostResponse>> {
  public async execute (req: DownvotePostDTO): Promise<DownvotePostResponse> {
    // member cannot be null  :)
    let member: Member;
    try {
      try {
       // This could be null  :(
        member = await this.memberRepo.getMemberByUserId(req.userId);
      } catch (err) {
        return left(new DownvotePostErrors.MemberNotFoundError())
      }
      return right(Result.ok<void>())

    } catch (err) {
      return left(new AppError.UnexpectedError(err));
    }
  }
}

Link to use case.

Having read this post I am thinking we should not be checking for null on the returns from the repos, also as this is often a value which signifies NotFound by ORMs such as Prisma.

Sorry for the long babble, but I would like to understand what the control / error handling flow should look like when Entity creation fails.

Thanks in advance!

[stemmlerjs]

Hey @ydennisy, thanks for opening the issue. I totally agree with you. I've been meaning to wrap back around to this and clean it up when I progress back up to Part IX in the book but you're completely right, this approach is problematic. We don't want to be implicitly handling nulls and leaving that to the application layer to deduce what that means. We should be as explicit as possible.

I think this problem originated from the reason that the word Result doesn't really seem like the correct word to describe something being found or not found. The word Maybe feels more appropriate. Perhaps we can redesign this abstraction.

The semantics of how we handle this and the abstraction that we use can differ, but in principle we want:

• To force the client to explicitly handle cases of the entity being found vs. it not being found
• Type guards on both the found and not found states

If I was to redesign the Result API, I'd still like to use it the way I've been using it, but I want separate methods and type guards depending on if its:

• a success/failure type of result (outcome) or
• a found/not-found type of result (search)

Taking a stab at the new API, here's what I put together:

abstract class _Result<T> {
  protected error?: string;
  protected _value?: T;

  constructor (error?: string, _value?: T) {
    this.error = error;
    this._value = _value;
  }

  public getErrorValue (): string {
    return this.error as string;
  }

  abstract getValue () : T | undefined;
}

class SuccessOrFailureResult<T> extends _Result<T> {
  public isSuccess: boolean;
  public isFailure: boolean;

  constructor (isSuccess: boolean, error?: string, value?: T) {
    super(error, value);
    this.isSuccess = isSuccess;
    this.isFailure = !isSuccess;
    Object.freeze(this);
  }

  public getValue () : T | undefined {
    if (!this.isSuccess) {
      console.log(this.error,);
      throw new Error("Can't get the value of an error result.")
    } 

    return this._value;
  }
}

class SearchResult<T> extends _Result<T> {
  public isFound: boolean;
  public isNotFound: boolean;

  constructor (isFound: boolean, error?: string, value?: T) {
    super(error, value);
    this.isFound = isFound;
    this.isNotFound = !isFound;
    Object.freeze(this);
  }

  getValue () : T | undefined {
    if (!this.isFound) {
      console.log(this.error,);
      throw new Error("Can't get the value of an error result.")
    } 

    return this._value;
  }
}

class Result {

  private static createSuccessOrFailureResult<T> (success: boolean, error?: string, value?: T) {
    if (success && error) {
      throw new Error("InvalidOperation: A result cannot be successful and contain an error");
    }
    if (!success && !error) {
      throw new Error("InvalidOperation: A failing result needs to contain an error message");
    }

    return new SuccessOrFailureResult<T>(success, error, value)
  }

  private static createSearchResult<T> (found: boolean, error?: string, value?: T) {
    if (found && error) {
      throw new Error("InvalidOperation: A result cannot be successful and contain an error");
    }
    if (!found && !error) {
      throw new Error("InvalidOperation: A failing result needs to contain an error message");
    }

    return new SearchResult<T>(found, error, value);
  }

  public static combineSearchResults<T> (results: SearchResult<T>[]) : SearchResult<T> {
    for (let result of results) {
      if (result.isNotFound) return result;
    }
    return Result.found<T>() as SearchResult<T>;
  }

  public static combineSuccessFailureResults<T> (results: SuccessOrFailureResult<T>[]) : SuccessOrFailureResult<T> {
    for (let result of results) {
      if (result.isFailure) return result;
    }
    return Result.ok<T>() as SuccessOrFailureResult<T>;
  }

  public static ok<U> (value?: U) : SuccessOrFailureResult<U> {
    return Result.createSuccessOrFailureResult<U>(true, undefined, value);
  }

  public static fail<U> (error: string): SuccessOrFailureResult<U> {
    return Result.createSuccessOrFailureResult<U>(false, error, undefined);
  }

  public static found<U> (value?: U) : SearchResult<U> {
    return Result.createSearchResult<U>(true, undefined, value);
  }

  public static notFound<U> (error: string): SearchResult<U> {
    return Result.createSearchResult<U>(false, error, undefined);
  }
}

type Maybe<T> = SearchResult<T>;
type SuccessOrFailure<T> = SuccessOrFailureResult<T>;

class Member {
  id: string;
  name: string;

  constructor (name: string) {
    this.id = '5'; // Generate uuid
    this.name = name;
  }
}

let repo = {
  getMember: function (id: string) : Maybe<Member> {
    let members = [{ id: '1', name: 'Bill'}]
    let aMember = members.find((m) => m.id === id);

    if (aMember) return Result.found<Member>(aMember);
    return Result.notFound<Member>(`Member with ${id} not found.`);
  },
  createMember: function (name: string) : SuccessOrFailure<Member> {
    if (!name) return Result.fail<Member>('Name not provided');
    return Result.ok<Member>(new Member(name));
  }
}

// Demonstration of found/not-found result logic 
let searchedMember: Maybe<Member> = repo.getMember ('2');

if (searchedMember.isFound) {
  console.log('member found!', searchedMember.getValue())
} 

if (searchedMember.isNotFound) {
  console.log('member not found', searchedMember.getErrorValue())
}

// Demonstration of success/failure result logic 
let createdMember: SuccessOrFailure<Member> = repo.createMember('khalil');

if (createdMember.isSuccess) {
  console.log('created member', createdMember.getValue())
} 

if (createdMember.isFailure) {
  console.log('did not create member', createdMember.getErrorValue())
}

Here's a link to a TS playground to mess around with the current design.

Usage could look like:

export class DownvotePost implements UseCase<DownvotePostDTO, Promise<DownvotePostResponse>> {
  public async execute (req: DownvotePostDTO): Promise<DownvotePostResponse> {
    // member cannot be null  :)
    let member: Maybe<Member>;

    try {

     member = await this.memberRepo.getMemberByUserId(req.userId);

     if (!member.isFound) {
       return left(new DownvotePostErrors.MemberNotFoundError())
     }

     // Continue
     member.getValue()
     ... 

    } catch (err) {
      return left(new AppError.UnexpectedError(err));
    }
  }
}

I'm curious to hear your thoughts!

[ydennisy]

Hey @stemmlerjs thanks for your reply! Really appreciate that!

I think this is a good addition, as right now we map to a MemberNotFound just with a try/catch and by throwing a generic error in the repo.

But my question was actually about another occurrence of null, and that is the one which can originate when creating a domain entity, expanding on the original pseudo code:

// repo.ts

const members = await.memberModel.findAll();
if (members === null) throw new Error('Not found');
return members.map(MemberMap.toDomain);

The return value is (Member | null)[] due to the this line in the mapper. This to me seems like the correct approach for the mapper - but I am not sure where and how best to handle the possible nulls from the mappers?

[ydennisy]

Ping @stemmlerjs