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

Defaulted values are not correctly copied for record type data. #1238

Closed
MaddyGuthridge opened this issue May 24, 2024 · 6 comments
Closed

Comments

@MaddyGuthridge
Copy link

When providing a default value for a record-type struct, the default value is not properly copied, meaning that it is at risk of subtle pass-by-reference bugs.

Consider this example:

import { create, defaulted, record, string } from 'superstruct';

// BAD: array is passed by reference
// =================================
const DefaultedRecord = defaulted(
  record(string(), string()),
  {},
);

const recordA = create(undefined, DefaultedRecord);
const recordB = create(undefined, DefaultedRecord);

recordA.name = 'maddy';

console.log(recordB.name);
// Expect: undefined
// Receive: "maddy"

// OK: object is created through function
// ======================================

const DefaultedRecordFn = defaulted(
  record(string(), string()),
  () => ({}),
);

const recordFnA = create(undefined, DefaultedRecordFn);
const recordFnB = create(undefined, DefaultedRecordFn);

recordFnA.name = 'george';

console.log(recordFnB.name);
// Expect: undefined
// Receive: undefined

The current behaviour of a defaulted(record(...), ...) is different compared to that of defaulted(object(...), ...) and defaulted(array(...), ...) which both appear to copy their input data.

@yeoffrey
Copy link
Contributor

yeoffrey commented Jun 23, 2024

Thanks for the report @MaddyGuthridge! I've tested this locally and I get the same issue.

After testing this a bit I think the easiest solution for this is to call structredClone on the default value in the defaulted coercion if its an object. For primatives this shouldn't happen because of the way that JS handles assignments.

export function defaulted<T, S>(
  struct: Struct<T, S>,
  fallback: any,
  options: {
    strict?: boolean
  } = {}
): Struct<T, S> {
  return coerce(struct, unknown(), (x) => {
    // To avoid a pass-by-reference bug, we'll clone objects when encountered
    // here, but the for performance avoid cloning primatives and functions
    const f =
      typeof fallback === 'function'
        ? fallback()
        : typeof fallback === 'object'
          ? structuredClone(fallback)
          : fallback

WDYT? cc: @arturmuller

@arturmuller
Copy link
Collaborator

@MaddyGuthridge thank you for the report!

@yeoffrey hmm. structuredClone destroys an object's prototype chain, meaning that if someone uses some custom class as the default value, they will unexpectedly get a plain object on the other end.

Using a custom class is probably a very rare use case, but also doesn't seem impossible.

I personally exclusively use POJOs with Superstruct, but... I guess someone somewhere could be doing something that this change could completely break.

I am honestly just leaning towards a warning in the docs, pointing people towards using the factory function signature for non primitive objects. My guess is that it was added exactly for this reason, just not properly documented.

@yeoffrey
Copy link
Contributor

@arturmuller Alternatively we could accept only a callback function. That would be a breaking change but to me this feels like a bit of a foot gun 😅

Up to you!

@MaddyGuthridge
Copy link
Author

I may actually have a suggestion for this: a library maintained by one of my coworkers jewire has some code that it uses to clone JS objects whilst preserving prototype chains (link to the source code). Perhaps you could use something similar?

@yeoffrey
Copy link
Contributor

I may actually have a suggestion for this: a library maintained by one of my coworkers jewire has some code that it uses to clone JS objects whilst preserving prototype chains (link to the source code). Perhaps you could use something similar?

This could be a good solution. I could try modifying #1248 to include that code. Thanks for the link!

@arturmuller
Copy link
Collaborator

This has been fixed by changes introduced by #1151 which was just released in v2.0.2. Thank you for the bug report!

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 a pull request may close this issue.

3 participants