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

Filter extra fields in interface decoder #147

Closed
lostintime opened this issue Mar 3, 2018 · 6 comments
Closed

Filter extra fields in interface decoder #147

lostintime opened this issue Mar 3, 2018 · 6 comments
Milestone

Comments

@lostintime
Copy link
Collaborator

Hello @gcanti, thanks for writing this great piece of software!

Here is an issue I discovered while solving similar problems:

Passing extra fields, which are not not defined by t.interface through decode method may turn into unexpected behavior at runtime when it's output assigned to other types with optional fields.

Here is a example:

import * as t from "io-ts"

// We have a response from external service
// Originally it was returning only `username`, `time` field was added later, 
//   which is not breaking compatibility
const serviceResponse: string = JSON.stringify({
  username: "lostintime",
  time: "2018-03-03 14:43:37"
})

// our type definition for service response
const UserProfile = t.interface({
  username: t.string
})

// one more type, using it somewhere deeper, adds a timestamp to user profile
type UserProfileWithTime = {
  username: string
  time?: number // timestamp (seconds)
}

// Parsing response
UserProfile.decode(JSON.parse(serviceResponse))
  .fold(
    e => {
      console.log("Something went wrong", e)
    },
    profile => {
      console.log("User profile is valid:", profile)

      // ... deeper in the call stack
      const prof: UserProfileWithTime = profile
      if (prof.time !== undefined) {
        // UserProfileWithTime.time must be a number
        console.log("1 hour later:", prof.time + 3600) // <<<<< time not a number!
      }
    }
  )

Output:

User profile: { username: 'lostintime', time: '2018-03-03 14:43:37' }
1 hour later: 2018-03-03 14:43:373600

Using strict interfaces is not a desired behavior, for given example, as our client will start to fail when external service deploys new time field in the response.

Filtering extra fields will of course have a performance impact, but to me - current behavior is very dangerous to leave as default.

While decode name implies a change/transformation of the input - changing it this way may not
count as "breaking". validate name is trickier, but it leads to same issue.

@gcanti
Copy link
Owner

gcanti commented Mar 3, 2018

@lostintime thanks for opening this issue.

The following cast is unsound

const prof: UserProfileWithTime = profile

though is allowed by TypeScript (which is unfortunate, AFAIK Flow complains here).

I think that there are (at least) two solutions to this problem.

(1) Never cast when optional fields are involved, refine instead

For maximum type safety profile should be refined. Fortunately io-ts runtime types provide custom type guards almost for free

const UserProfileWithTime = t.intersection([UserProfile, t.partial({ time: t.number })])

...

    // ... deeper in the call stack
    if (UserProfileWithTime.is(profile)) {
      if (profile.time !== undefined) {
        // ...
      }
    }

(2) You are willing to pay for the additional cost of stripping the extra fields

Then you could define a strip combinator

export function strip<P extends t.Props, A, O>(type: t.InterfaceType<P, A, O>): t.InterfaceType<P, A, O> {
  const keys = Object.keys(type.props)
  const len = keys.length
  return new t.InterfaceType(
    type.name,
    type.is,
    (m, c) =>
      type.validate(m, c).map((o: any) => {
        const r: any = {}
        for (let i = 0; i < len; i++) {
          const k = keys[i]
          r[k] = o[k]
        }
        return r
      }),
    type.encode,
    type.props
  )
}

// our type definition for service response
const UserProfile = strip(
  t.interface({
    username: t.string
  })
)

p.s.
interface cannot strip extra fields because it would

  • add a performance hit
  • be a breaking change (even if a desiderated one)
  • break t.intersection

@lostintime
Copy link
Collaborator Author

(1) Never cast when optional fields are involved, refine instead

The problem I see with solution this is that I should care about it "deeper in the call stack", and it also adds a runtime cost by calling UserProfileWithTime.is(profile) in every place I need it.

I personally prefer to cover my back at inputs and safely play with plain types after. Also you may not be able to control that assignment of structurally compatible types - when happens in 3rd parties.

Using just t.interface (which I'd use by default) creates a false sens of safety :(.

(2) You are willing to pay for the additional cost of stripping the extra fields

Then you could define a strip combinator

This looks like a reasonable solution! Maybe it can be added to the library?

Also it may be worth mentioning this case somewhere near t.interface documentation

Thanks!

@gcanti
Copy link
Owner

gcanti commented Mar 3, 2018

I personally prefer to cover my back at inputs and safely play with plain types after

I mean, that kind of cast is always unsound, whether you are using io-ts or not.

When using io-ts you can increase the likelihood of not messing up but, as you noticed, you can't really know what happens deeper in the call stack between the decoding and the cast

(contrived example)

type UserProfile = {
  username: string
}

type UserProfileWithTime = {
  username: string
  time?: number // timestamp (seconds)
}

type UserProfileWithISOTime = {
  username: string
  time?: string // timestamp (ISO)
}

UserProfile.decode(JSON.parse(serviceResponse)).fold(
  e => {
    console.log('Something went wrong', e)
  },
  profile => {
    console.log('User profile is valid:', profile)

    // ... deeper in the call stack
    const x: UserProfileWithISOTime = { ...profile, time: 'foo' }
    // ... deeper in the call stack
    const y: UserProfile = x

    // ... deeper in the call stack
    const prof: UserProfileWithTime = y // opsss...
    if (prof.time !== undefined) {
      console.log('1 hour later:', prof.time + 3600) // 1 hour later: foo3600
    }
  }
)

Obviously you can judge better than me if stripping at the boundaries is safe enough in your use case, I just wanted to point out that there's always a risk ;)

This looks like a reasonable solution! Maybe it can be added to the library?

Yes, maybe to io-ts-types though

Also it may be worth mentioning this case somewhere near t.interface documentation

Sure, would you like to send a PR?


Btw for a discussion about a type safe pattern matching, check out runtypes/runtypes#36 (comment)

@lostintime
Copy link
Collaborator Author

You're absolutely right! it doesn't matter where that hidden fields comes from, io-ts or other cast - it can break things anyway, and mutability makes it even worse #13347, must be always careful.

Maybe this will help #12936 - i it still applies to optional fields.

This is probably what you pointed to flow about: https://flow.org/en/docs/types/objects/#toc-exact-object-types - I never used flow before, you have to use a special syntax, but there is a solution at least. It would be great if tsc can at least warn about such casts (

Sure, would you like to send a PR?

Yes! I will

Thanks again for opening my eyes )

@gcanti gcanti added this to the 2.0 milestone Jan 6, 2019
@gcanti gcanti modified the milestones: 2.0, 1.8 Feb 2, 2019
@gcanti
Copy link
Owner

gcanti commented Feb 2, 2019

@lostintime will #277 fix the original issue?

@lostintime
Copy link
Collaborator Author

@lostintime will #277 fix the original issue?

@gcanti yep, looking at the tests from #277 seems like t.exact(t.type(...)), t.strict(...), t.exact(t.partial(...)) and even t.exact(t.intersection(...)) works as expected.

Thanks for the great job! 👍

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

No branches or pull requests

2 participants