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

feat: add basic field type runtime validators #113

Merged
merged 4 commits into from
Feb 8, 2021

Conversation

wschurman
Copy link
Member

@wschurman wschurman commented Jan 29, 2021

Why

We're seeing a decent number of bugs/crashes surrounding invalid types of values being set in mutators and invalid types of load-by values being used in loaders.

These bugs manifest themselves in obvious ways sometimes, and other times less obvious.

  • An example on read might look like: select * from "table" where "id" = ANY($1) - invalid input syntax for uuid: "c6cef326-2a93-4477-8e46-7faae8e1684"
  • An example on write looks like: TypeError: Cannot read property 'push' of undefined

The write example is a fairly clear error already, but this PR preempts the round trip to the DB by detecting a query that will most likely fail anyways ahead of time.

For the read example, the error is more cryptic and is what triggered the investigation and this PR as the result. The issue is that when a load comes in for a UUID that postgres thinks is valid, but postgres returns the UUID in the normalized format in the result set, we don't know to create a map entry at https://github.com/expo/entity/blob/master/packages/entity/src/EntityDatabaseAdapter.ts#L123 so the push call a few lines later will fail. Concretely, the trace is:

  • Request comes in for UUID a0eebc999c0b4ef8bb6d6bb9bd380a11 which postgres thinks is valid.
  • The code in entity creates a map entry for a0eebc999c0b4ef8bb6d6bb9bd380a11.
  • Postgres returns row with normalized UUID a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11.
  • We look in the map expecting the array to exist but it doesn't since they're different strings.

Note that this postgres behavior is documented at https://www.postgresql.org/docs/9.1/datatype-uuid.html

How

Add field value validation to the following calls:

  • EntityMutator.createAsync/updateAsync
  • EntityLoader.loadByFieldValueAsync and similar methods.

Test Plan

Run tests, including new ones to test behavior and validation correctness.

@codecov
Copy link

codecov bot commented Jan 29, 2021

Codecov Report

Merging #113 (a265077) into master (5a8479f) will increase coverage by 0.12%.
The diff coverage is 97.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
+ Coverage   94.96%   95.08%   +0.12%     
==========================================
  Files          66       67       +1     
  Lines        1627     1689      +62     
  Branches      193      202       +9     
==========================================
+ Hits         1545     1606      +61     
- Misses         80       81       +1     
  Partials        2        2              
Flag Coverage Δ
integration 95.08% <97.53%> (+0.12%) ⬆️
unittest 95.08% <97.53%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/entity/src/EntityCompanion.ts 100.00% <ø> (ø)
packages/entity/src/EntityLoader.ts 88.76% <90.47%> (+1.26%) ⬆️
...-adapter-knex/src/PostgresEntityDatabaseAdapter.ts 100.00% <100.00%> (ø)
packages/entity/src/EntityConfiguration.ts 100.00% <100.00%> (ø)
packages/entity/src/EntityDatabaseAdapter.ts 100.00% <100.00%> (ø)
packages/entity/src/EntityFields.ts 100.00% <100.00%> (ø)
packages/entity/src/EntityLoaderFactory.ts 100.00% <100.00%> (ø)
packages/entity/src/EntityMutator.ts 98.14% <100.00%> (+0.14%) ⬆️
packages/entity/src/errors/EntityError.ts 100.00% <100.00%> (ø)
.../entity/src/errors/EntityInvalidFieldValueError.ts 100.00% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a8479f...9077866. Read the comment docs.

@wschurman wschurman marked this pull request as ready for review January 29, 2021 05:34
packages/entity/src/EntityFields.ts Outdated Show resolved Hide resolved
packages/entity/src/EntityFields.ts Outdated Show resolved Hide resolved
@@ -1,3 +1,5 @@
import { validate as validateUUID } from 'uuid';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, didn't know this existed, we should use it in more places where we currently write out the regex ourselves

packages/entity/src/EntityMutator.ts Outdated Show resolved Hide resolved
packages/entity/src/EntityLoader.ts Outdated Show resolved Hide resolved
packages/entity/src/EntityLoader.ts Outdated Show resolved Hide resolved
Copy link
Member

@quinlanj quinlanj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @ide about the AggregateError.

It'd make sense to throw an InvalidFieldValueError if www is not able to fix or detect the error without making a trip to Postgres. An example of this is corrupt data in postgres itself.

But in this case, it should be the responsibility of www to make sure the field inputs are correct. The entity framework should throw an error without wrapping it in a result, and only request data from the DB when it knows all the inputs are well formed.

We also discussed this on zoom, and found out dataloaders has this behaviour, so this will be a happy case of accidental consistency lol

@wschurman
Copy link
Member Author

wschurman commented Feb 6, 2021

@quinlanj and I talked a bit about this and think that throwing independently of Result actually makes the most sense for two reasons:

  • This is in some sense programmer error (passing an invalid value into the framework). The framework returns Result<Error> only for things that are valid framework calls but yield either not found or not authorized. For things like database misconfiguration (
    `loadByFieldEqualing: Multiple entities of type ${this.entityClass.name} found for ${uniqueFieldName}=${fieldValue}`
    ) we throw an error instead.
  • The current behavior is actually to throw an error for the cases specified in the summary.
  • Results are best for isolating failures so that other items in a batch may succeed. In the case of invalid field values in a load for example though, it would cause the whole batch to fail before this fix, so it's fine to preemptively fail the batch. For invalid field values in write it would also throw and failing the write preemptively is also fine.

I'll update the PR to reflect this.

@wschurman wschurman merged commit 6c4d4b0 into master Feb 8, 2021
@wschurman wschurman deleted the @wschurman/field-validation branch February 8, 2021 17:36
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.

3 participants