data-client icon indicating copy to clipboard operation
data-client copied to clipboard

RFC: Better automatic handling of Summary/Extended Entities

Open ntucker opened this issue 2 years ago • 1 comments

Background

Currently the https://resthooks.io/docs/getting-started/validation#partial-results case is a rather cumbersome endeavor; requiring users to maintain custom validation methods for each Resource/Entity they define. Furthermore, in some systems like GraphQL having partial results is quite common.

What makes this more problematic is doing this incorrectly can lead to serious bugs, where data is missing when it is expected. For most cases the default fields provide information about which fields are expected. However, fields can be optional. Unfortunately in these cases, the default value isn't always something that is obvious like null. For instance here, a user had an API that sometimes had a Date field. However, the default case the date would be there so it made more sense to have an actual Date default.

While we have made it easier to 'opt-out' of these sorts of validations, it has become clear that doing this out of the box is not intuitive behavior.

Properties of the critical use-case

class SummaryAnalysis extends Resource {
  readonly id: string = '';
  readonly createdAt: Date = new Date(0);
  readonly meanValue: number = 0;
  readonly title: string = '';

  pk() {
    return this.id;
  }
}

class FullAnalysis extends SummaryAnalysis {
  readonly graph: number[] = [];
  readonly lastRun?: Date = new Date(0);

  static schema = {
    lastRun: Date,
  }
}
  • has a non-abstract parent class
  • that parent class has the same key

In this case, if we did not have validation it could lead to serious issues as graph may not exist when we expect it to. On the other hand, lastRun is optional, so if it is missing this is fine.

Solution - make opt-in easier

We can add a simple helper to make defining these easy. Even better - if someone does not have optional fields, the defaults static member can be used to add this to a base class. This is great because it still allows opt-out, while having the default behavior more protective. And since the user explicitly added this to the base class the behavior should not be as surprising.

class CustomBaseResource extends Resource {
  static validate(processedEntity) {
    return validateRequired(processedEntity, this.defaults) || super.validate(processedEntity);
  }
}
class FullAnalysis extends SummaryAnalysis {
  readonly graph: number[] = [];
  readonly lastRun?: Date = new Date(0);

  static schema = {
    lastRun: Date,
  }

  static validate(processedEntity) {
    return validateRequired(processedEntity, exclude(this.defaults, ['lastRun']));
  }
}

Solution - add field validation metadata

class FullAnalysis extends SummaryAnalysis {
  readonly graph: number[] = [];
  readonly lastRun?: Date = new Date(0);

  static schema = {
    lastRun: Date,
  }
  static fields = {
    graph: compose(isRequired, isArray('number')),
    lastRun: isDate,
  }
}

Future work

Add babel pre-processor to to turn ? to validation information.

ntucker avatar Jun 03 '22 21:06 ntucker

Doc case, when my detail resource has fields that may all be "optional" (as in doesn't exist for the resource -- see example below, all users have firstName/lastName and some have nickName. In order to fetch the DetailResource need to mark nickName as required. For API design, this means the DetailResource.detail endpoint needs to explicitly include keys with null values in the case where the resource has no value for the key.

SummaryResouce {
  readonly firstName: string = ""
  readonly lastName: string = ""
}

DetailResource extends SummaryResouce {
  // This field may or may not exist in the resource, but still needs to be "required" so client fetches the detail endpoint for this resource
  readonly nickName: string = null 
}

sweetro avatar Jul 29 '22 16:07 sweetro