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

fix(mongoose): Fix json schema for VirtualRef if MongooseModels entry not set yet #1672

Merged
merged 2 commits into from
Dec 7, 2021

Conversation

ochrstn
Copy link
Contributor

@ochrstn ochrstn commented Dec 4, 2021

Information

Type Breaking change
Fix No

Sadly my last fix (#1670) introduced a new bug.

MongooseModels.get(ref) can return undefined and the property doesn't even get serialized. With the fallback to Object it would behave like before.

That happens a lot in my project because

MongooseModels.set(collectionName, target);
gets executed after
return !isString(ref) ? ref : MongooseModels.get(ref);

@Romakita any idea on how to fix the order problem?

Todos

  • Tests
  • Coverage
  • Example
  • Documentation

@github-actions
Copy link

github-actions bot commented Dec 4, 2021

Benchmark result

tsed vs express: New benchmarks generated
tsed-koa vs koa: New benchmarks generated.

Note:
Average of all diffs for Ts.ED-* so: (0.15 + 0.14 + 0.12 + 0.12) / 4

Details:

Req/sec Trans/sec Req/sec DIFF Trans/sec DIFF
Ts.ED Express 1967 551.27KB - -
Ts.ED Koa 2325 463.24KB - -
Nest-Express 3085 719.93KB - -
Nest-Fastify 8005 1.34MB - -
Express 3485 813.45KB - -
Express Router 3413 796.61KB - -
Koa 8318 1.40MB - -
Fastify 11019 1.85MB - -

Note:
req/sec DIFF and Trans/sec DIFF is in comparison to the baseline on target branch (master).

@Romakita
Copy link
Collaborator

Romakita commented Dec 5, 2021

@ochrstn Maybe we can wrap the ref in arrow function instead ?

but the first step is a to add a unit or integration test that show the problem + expectation. Then we can find a solution.

See you
Romain

@github-actions
Copy link

github-actions bot commented Dec 5, 2021

Benchmark result

tsed vs express: New benchmarks generated
tsed-koa vs koa: New benchmarks generated.

Note:
Average of all diffs for Ts.ED-* so: (0.15 + 0.14 + 0.12 + 0.12) / 4

Details:

Req/sec Trans/sec Req/sec DIFF Trans/sec DIFF
Ts.ED Express 1733 485.70KB - -
Ts.ED Koa 2124 423.22KB - -
Nest-Express 2721 634.96KB - -
Nest-Fastify 6790 1.14MB - -
Express 2936 685.24KB - -
Express Router 2939 685.99KB - -
Koa 8050 1.35MB - -
Fastify 8631 1.45MB - -

Note:
req/sec DIFF and Trans/sec DIFF is in comparison to the baseline on target branch (master).

@ochrstn
Copy link
Contributor Author

ochrstn commented Dec 5, 2021

@ochrstn Maybe we can wrap the ref in arrow function instead ?

but the first step is a to add a unit or integration test that show the problem + expectation. Then we can find a solution.

See you Romain

I added a failing test.

I also tried to change the VirtualRef decorator so that it's more similar to the Ref

function getType(opts: any) {
  const ref = opts.ref || opts.type;
  return isString(ref) ? MongooseModels.get(ref) || Object : isArrowFn(ref) ? ref() : ref;
}

export function VirtualRef(options: string | MongooseVirtualRefOptions, foreignField?: string): Function {
  const opts = getInitialOpts(options, foreignField);
  const schema = mapToSchema(opts);
  return useDecorators(
    StoreMerge(MONGOOSE_SCHEMA, schema),
    JsonEntityFn((store) => {
      store.itemSchema.type(schema.count ? Number : lazyRef(getType(opts)));
      store.type = Object;
    })
  );
}

But that results "$ref": "#/definitions/JsonSchema" types for String references and other problems for Arrow function references

@Romakita
Copy link
Collaborator

Romakita commented Dec 7, 2021

Fixed :)

@github-actions
Copy link

github-actions bot commented Dec 7, 2021

Benchmark result

tsed vs express: New benchmarks generated
tsed-koa vs koa: New benchmarks generated.

Note:
Average of all diffs for Ts.ED-* so: (0.15 + 0.14 + 0.12 + 0.12) / 4

Details:

Req/sec Trans/sec Req/sec DIFF Trans/sec DIFF
Ts.ED Express 1810 507.43KB - -
Ts.ED Koa 2174 433.02KB - -
Nest-Express 2860 667.60KB - -
Nest-Fastify 7188 1.21MB - -
Express 3169 739.66KB - -
Express Router 3151 735.38KB - -
Koa 8412 1.41MB - -
Fastify 9859 1.65MB - -

Note:
req/sec DIFF and Trans/sec DIFF is in comparison to the baseline on target branch (master).

@Romakita Romakita merged commit 746e6d6 into production Dec 7, 2021
@Romakita Romakita deleted the fix-mongoose-vref-schema-2 branch December 7, 2021 06:51
@Romakita
Copy link
Collaborator

Romakita commented Dec 7, 2021

🎉 This PR is included in version 6.95.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Romakita pushed a commit that referenced this pull request Dec 29, 2021
…ry not set yet (#1672)

* fix(mongoose): Fix json schema for `VirtualRef` if MongooseModels entry not set yet

* fix(mongoose): Wrap type into an arrow function to lazy load the model
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants