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 Transform #191

Closed
wants to merge 5 commits into from
Closed

feat: add Transform #191

wants to merge 5 commits into from

Conversation

yuhr
Copy link
Collaborator

@yuhr yuhr commented Mar 15, 2021

BREAKING CHANGE: check method of compound runtypes such as Record and Tuple now returns a new object instead of original one

Resolves #56. It's inspired by #56 (comment). Detailed usage is in the tests. Above change is needed in order to make Transform work with those non-primitive runtypes. Now investigating possible side effects, bugs & design flaws...

Example usage:

const Input = Record({
  date: String.withTransform(s => {
    const date = new Date(s);
    if (isNaN(date.valueOf())) throw new Error(`Invalid date string "${s}"`);
    else return date;
  }),
});
type Input = Static<typeof Input>;
// { date: Date; }

Input.check({ date: 'crap' });
// ValidationError: Failed transform: Error: Invalid date string "crap" in date

const input: Input = Input.check({ date: '1990-01-01' });
console.log(input.date.toUTCString());
// Mon, 01 Jan 1990 00:00:00 GMT

BREAKING CHANGE: `check` method of compound runtypes such as `Record` and `Tuple` now returns a new object instead of original one
@coveralls
Copy link
Collaborator

coveralls commented Mar 15, 2021

Coverage Status

Coverage increased (+0.05%) to 99.155% when pulling 8d609a4 on yuhr:feat/transform into 38e9ace on pelotom:master.

@yuhr
Copy link
Collaborator Author

yuhr commented Mar 16, 2021

ForbesLindesay#16 (comment)

A side effect of this change is that we are also effectively deep cloning everything,

This is correct for us too.

which takes care of pelotom#132 and pelotom#24 (remove unknown keys from validated object).

I think this is still remaining to be considered in some point of view, that is, we could still include unknown properties in the resulting object, but yes, it would be better to remove in favor of these issues (and 52e8900 already does this). This way, we shall state that Record and Partial will always strip unknown properties of the resulting object, so if one wants to include them in the output, explicit runtypes are needed for them, for example intersecting Dictionary or somehow.

Additionally, this will consequently handle "exact" type matching #41. It feels like we're somewhat on the right track further than #162, as it won't work with Intersect or Union. Sorry this is a mistake, Records simply strip unknown properties silently, but "exact" types are supposed to emit an error then.

It may be worth looking into supporting a fast path that doesn't clone objects if none of the validations change anything.

I don't believe this would improve performance or memory consumption in any terms, because it requires us to introduce additional strict-equality check at every single runtype validation.

@yuhr
Copy link
Collaborator Author

yuhr commented Mar 16, 2021

Note that Transform won't work properly with decorators, as decorators currently don't support any transform of static types of their targets such as parameters. microsoft/TypeScript#4881

@yuhr
Copy link
Collaborator Author

yuhr commented Mar 17, 2021

The side effect that it removes unknown properties from validated records will also address the problem described in #111:

import { Record, String } from 'runtypes';

const User = Record({
  userName: String,
  displayName: String,
});

async function getUser(req, res, next): Promise<void> {
  ...
  const rawUser:  = await getUserFromDataBase();
  const typedUser = User.check(rawUser);

  res.body(JSON.stringify(myUser)); // DANGER DANGER could include property 'socialSecurityNumber'
}

This could be very dangerous if the object returned by getUserFromDataBase looked like this:

{
  userName: "maclockard",
  displayName: "Mac",
  socialSecurityNumber: 555555, // DANGER DANGER
}

Obviously this will already be resolved by this PR.

As I have written here, once we get Transform, the "stringify" feature proposed by #111 is easily created by having another runtype dedicated to serialization.

@yuhr yuhr linked an issue Mar 17, 2021 that may be closed by this pull request
@yuhr
Copy link
Collaborator Author

yuhr commented Mar 17, 2021

Well, I understand in some cases it's not "easily created", especially for complex Records. In those cases we might think of having some convenient way to generate a new runtype derived from existing one. This conversation is out of scope of this PR, since this PR is supposed to provide a basic functionality to perform data transformation.

@yuhr yuhr marked this pull request as ready for review March 17, 2021 20:47
@yuhr
Copy link
Collaborator Author

yuhr commented Mar 19, 2021

Oh this will break the semantics of guard and assert now, that needs a solution definitely.

@yuhr yuhr marked this pull request as draft March 19, 2021 03:52
@yuhr yuhr modified the milestones: Runtypes 6, Runtypes 7 Mar 25, 2021
@peterjwest
Copy link

Looking forward to this feature, will be super useful!

@peterjwest
Copy link

It is a shame the non-mutating behaviour will be lost though, since it's so low overhead.

@yuhr
Copy link
Collaborator Author

yuhr commented Apr 28, 2021

@peterjwest Sorry, my wording was improper. I said misleading terms such as "strip", "remove" and "side effect", but by these words I did mean just "make it not present" in the returned object (and its static type) from ".check()". It doesn't touch the argument, and will always return a new object instead of the original one. I don't want to introduce any form of mutating behaviour.

But that's why my current implementation makes guard and assert nonsense, i.e. Transform can change the Static type of a runtype, but guard and assert do not return the transformed value but just assure that the argument's type is the transformed one (Of course this behaviour is invalid, and needs to be resolved).

I think we would add a new type parameter like RuntypeBase<A, B = A> where B means the transformed type (different from A only in Transform runtypes), so that .guard() and .assert() assert the argument is A, while .check() returns B. But then I'm unsure what type should be returned by Static<typeof Transform<OriginalRuntype, TransformedStaticType>>...?

@peterjwest
Copy link

I think I understand what you're saying, and that sounds great! But in regard to your question, I have absolutely no idea.

@milankinen
Copy link

milankinen commented Jun 1, 2021

Any news about this? This feature would be very important to our team if it gets ready (especially if extra object keys will be dropped). So far we've used this kind of utility function but it feels a bit hacky and we'd rather implement this as real real feature to the core library:

const withoutExtraKeys = <T>(type: rt.Runtype<any>, value: any): T => {
  const reflect = type.reflect
  switch (reflect.tag) {
    case 'record':
      return Object.entries(reflect.fields).reduce((result, [propertyKey, propertyType]) => {
        if (propertyType.tag !== 'optional' || propertyKey in value) {
          result[propertyKey] = withoutExtraKeys(propertyType, value[propertyKey])
        }
        return result
      }, {} as T)
    case 'dictionary':
      const valueType = reflect.value
      return Object.entries(value).reduce((res, [key, value]) => {
        res[key] = withoutExtraKeys(valueType, value)
        return res
      }, {} as T)
    case 'array':
      const elementType = reflect.element
      return value.map((x) => withoutExtraKeys(elementType, x))
    case 'tuple':
      return reflect.components.map((componentType, idx) =>
        withoutExtraKeys(componentType, value[idx])
      ) as any
    case 'optional':
      return value === undefined ? value : withoutExtraKeys(reflect.underlying, value)
    case 'union':
      for (const altType of reflect.alternatives) {
        if (altType.validate(value).success) {
          return withoutExtraKeys(altType, value)
        }
      }
      return value
    case 'intersect':
      if (reflect.intersectees.length === 0) {
        return value
      } else {
        return reflect.intersectees.reduce((result, intersecteeType) => {
          const intersecteeValue = withoutExtraKeys(intersecteeType, value)
          if (_.isPlainObject(result) && _.isPlainObject(intersecteeValue)) {
            Object.assign(result, intersecteeValue)
            return result
          } else {
            return intersecteeValue
          }
        }, {} as any)
      }
    default:
      return value
  }
}

export const checkNoExtraKeys = <T>(type: rt.Runtype<T>, value: unknown): T => {
  type.check(value)
  return withoutExtraKeys<T>(type, value)
}

I can also help with this PR if it helps to get it closer to being merged!

@peterjwest
Copy link

Here's my version of the same thing.
It's not as feature complete as yours, but mine returns the original object unless it would be mutated.

import { mapValues, pickBy } from 'lodash';
import * as runtypes from 'runtypes';

export default function restrict<Data, RunType extends runtypes.Runtype<Data>>(RunType: RunType, data: Data) {
  return restrictInternal(RunType, data) || data;
}

function restrictInternal<Data, RunType extends runtypes.Runtype<Data>>(
  RunType: RunType, data: Data,
): Data | undefined {
  const reflected = RunType.reflect;
  if (reflected.tag === 'record') {
    let mutated = false;

    const newData: Data = mapValues(pickBy(data as any, (item, key) => {
      const hasKey = key in reflected.fields;
      if (!hasKey) mutated = true;
      return hasKey;
    }), (item, key) => {
      const newItem = restrictInternal(reflected.fields[key], item);
      if (newItem !== undefined) mutated = true;
      return newItem || item;
    });

    if (mutated) return newData;
  }
  if (reflected.tag === 'array') {
    let mutated = false;

    const newData: Data = (data as any).map((item: any) => {
      const newItem = restrictInternal(reflected.element, item);
      if (newItem !== undefined) mutated = true;
      return newItem || item;
    });

    if (mutated) return newData;
  }
  if (reflected.tag === 'dictionary') {
    let mutated = false;

    const newData: Data = mapValues(data as any, (item) => {
      const newItem = restrictInternal(reflected.value, item);
      if (newItem !== undefined) mutated = true;
      return newItem || item;
    });

    if (mutated) return newData;
  }

  if (reflected.tag === 'union') {
    const alternative = reflected.alternatives.find((alternative) => {
      try {
        alternative.check(data);
        return true;
      } catch {
        return false;
      }
    });
    return restrictInternal(alternative as any, data);
  }

  if (reflected.tag === 'optional') {
    return restrictInternal(reflected.underlying as any, data);
  }

  return undefined;
}

@jgrigg
Copy link

jgrigg commented Jun 19, 2021

If we are transforming in the direction of Json => TS is there then also appetite for transformations in the opposite direction i.e TS => Json? Presumably we'd need a new function like encode() à la io-ts

@WilliamABradley
Copy link

WilliamABradley commented Jan 19, 2022

@yuhr design-wise, shouldn't withTransform be inverted? Or am I thinking more on the lines of a withConverter function.

Ideally my thought is that I have:

export const DateType = rt.InstanceOf(Date).withConverter((val: unknown) => {
  if (typeof value === 'string') {
    value = new Date(DateString.check(value));
  }
  if (DateTime.isDateTime(value)) {
    value = value.toJSDate();
  }
  return value;
});
export type DateType = rt.Static<typeof DateType>;

That way it can deal with any kind of unknown value, and try and turn it into the Date Type, and if it fails, throw a validation error. E.g. handle parsing of Luxon objects too.

To represent this with your withTransform function, do I use a union of possible input types?

E.g.

export const DateType = rt.Union(rt.InstanceOf(Date), rt.InstanceOf(DateTime), rt.String)
  .withTransform((val: Date | DateTime | string) => {
      if (typeof value === 'string') {
        value = new Date(DateString.check(value));
      }
      if (DateTime.isDateTime(value)) {
        value = value.toJSDate();
      }
      return value;
});
export type DateType = rt.Static<typeof DateType>;

I have implemented simplisitc parsing functions for types and records in this comment: #56 (comment)

With covers the parsing and stripping unknown field use cases, but having it built into the library would be a lot nicer.

@yuhr
Copy link
Collaborator Author

yuhr commented Dec 9, 2023

Closing as too stale; I'll file another PR for this.

@yuhr yuhr closed this Dec 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment