Attempting to access Result value in possible error case.

[jcorkhill]

In this commit (lines 27, 28) for your UserMap.ts mapper, you are attempting to access the Result value from the ValueObjects when they might not necessarily have been created properly:

 public static toDomain (raw: any): User {
    const userEmailOrError = UserEmail.create(raw.user_email);
    const userPasswordOrError = UserPassword.create(raw.user_password);

    const userOrError = User.create({
      email: userEmailOrError.getValue(),
      password: userPasswordOrError.getValue(),
      firstName: raw.first_name,
      lastName: raw.last_name,
      isEmailVerified: raw.is_email_verified,
      username: raw.username
    }, new UniqueEntityID(raw.base_user_id))

    userOrError.isFailure ? console.log(userOrError.error) : '';

    return userOrError.isSuccess ? userOrError.getValue() : null;
  }

If either UserEmail.create(): Result<UserEmail> or UserPassword.create(): Result<UserPassword> fail, then you'll see the following error when you call getValue on the Result instance:

"Can't get the value of an error result. Use 'errorValue' instead."

I'm interested in your architecture, so I was wondering if this is something you intend to rectify, and if so, how you'll do it.

Might you combine the results, perhaps:

 public static toDomain (raw: any): User {
    const userEmailOrError = UserEmail.create(raw.user_email);
    const userPasswordOrError = UserPassword.create(raw.user_password); 
    
    if (!Result.combine([userEmailOrError, userPasswordOrError])) return null;

    const userOrError = User.create({
      email: userEmailOrError.getValue(),
      password: userPasswordOrError.getValue(),
      firstName: raw.first_name,
      lastName: raw.last_name,
      isEmailVerified: raw.is_email_verified,
      username: raw.username
    }, new UniqueEntityID(raw.base_user_id))

    userOrError.isFailure ? console.log(userOrError.error) : '';

    return userOrError.isSuccess ? userOrError.getValue() : null;
  }

Or, are you asserting that both value objects will never be erroneous because they are coming straight from the database where the integrity and validation of data are guaranteed?

Thank you.

[stemmlerjs]

Yeah, that's definitely been something I've been tip-toeing around a little bit.

I think we should be adhering to the fail fast principle here. While it might not be perfect, I think we should explicitly throw an error here.

 public static toDomain (raw: any): User {
    const userEmailOrError = UserEmail.create(raw.user_email);
    const userPasswordOrError = UserPassword.create(raw.user_password); 
    const combinedPropsResult = Result.combine([userEmailOrError, userPasswordOrError]);
    
    if (!combinedPropsResult.success) {
       throw new Error(combinedPropsResult.message)
    }

    const userOrError = User.create({
      email: userEmailOrError.getValue(),
      password: userPasswordOrError.getValue(),
      firstName: raw.first_name,
      lastName: raw.last_name,
      isEmailVerified: raw.is_email_verified,
      username: raw.username
    }, new UniqueEntityID(raw.base_user_id))

    return userOrError.isSuccess ? userOrError.getValue() : null;
  }

I'll explain why.

In this article, I said:

Typically, we should aim to reserve try-catch blocks for operations that interact with the outside world (ie: we're not the ones throwing errors), like:

Operations against a database
Operations utilizing an external service (API, etc)

My rationale for this refactoring is because we're treating working against something volatile, like the shape of the database table, we're already mistrusting what could come back.

Here's an example of failing fast that I did in the DDDForum app.

export class MemberRepo implements IMemberRepo {
  private models: any;

  constructor (models: any) {
    this.models = models;
  }
... 
  public async getMemberIdByUserId (userId: string): Promise<MemberId> {
    const MemberModel = this.models.Member;
    const baseQuery = this.createBaseQuery();
    baseQuery.where['member_base_id'] = userId;
    const member = await MemberModel.findOne(baseQuery);
    const found = !!member === true;
    if (!found) throw new Error('Member id not found');
    return MemberIdMap.toDomain(member);
  }
}

In this example, the MemberRepo.getMemberIdByUserId will throw an error instead of returning null, because it's not helpful to return null. That means the calling code needs to know that the API could return null or it could return a MemberId.

My personal preference here is to treat anything that interacts with infrastructure as volatile and throw errors when we get undesirable behavior, and functionally handle typed errors within the application and domain layer, since it encapsulates critical business logic where the absence of a Member might actually be relevant business logic. Like:

export namespace GetMemberByUserNameErrors {

  export class MemberNotFoundError extends Result<UseCaseError> {
    constructor (username: string) {
      super(false, {
        message: `Couldn't find a member with the username ${username}`
      } as UseCaseError)
    }
  }
}

I don't think this is perfect, but it's adequate and backed by what I think is reasonable logic.