-
Notifications
You must be signed in to change notification settings - Fork 761
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: deep array merge #2324
fix: deep array merge #2324
Conversation
fc25601
to
a36bcc3
Compare
@lipnitsk - unfortunately, I've moved on from the previous job where we had this spec, so I don't have ready access to test. I've reached out to some former coworkers to see if they can send me a sample - but will also check my personal archive to see if I have a copy lying around. |
@dballance thanks! I also noticed that deepmerge has a |
I've got a copy of the json OpenAPI spec that is massive. Gonna try to get it tested this week - and also see if I can rework it to remove identifying features so it can be added as a test in the repo here. |
@lipnitsk Finally got some time to sit down and test this. Everything appears to be working with this spec, with a slight regression in resolution speed.
So, ~44% regression in spec resolution speed. Seems fine to me given the nature of the size of the spec - just wanted to provide the data so the maintainers have it for reference. The only comment I would have is that both Spec Stats
VerificationTo verify, I ran the below test on both branches (
Testdescribe('huge spec', () => {
let spec;
beforeAll(() => {
spec = require('./data/huge.json');
});
test('should resolve', async () => {
console.time('huge');
const first = await Swagger({ spec });
console.timeEnd('huge');
expect(first.spec.info.title).toEqual('something');
fs.writeFileSync('./huge-parsed.json', JSON.stringify(first.spec, null, 2), 'utf-8');
});
}); |
@dballance agreed. If performance becomes an issue it can be optimized, but your test case is probably one of the worst. Until such need arises, keeping things simple is probably best. @char0n can we merge this fix? Thanks for testing and such a detailed response, btw! |
25e6d47
to
4d81f82
Compare
@char0n, can we merge this please? |
@lipnitsk sorry for late response. Looking into this now |
Firstly thank you for contributing and collaborating on this issue/PR. Replacing const { mergeWith } = require('lodash/fp')
const merge = mergeWith((objValue, srcValue) => {
if (Array.isArray(objValue)) {
if (!Array.isArray(srcValue)) {
return srcValue;
}
return objValue.concat(srcValue)
}
});
merge(target, source); // immutable merge I've run this Regarding performance - it has been discussed in this PR that there is a performance degradation when using
for the following definition: {
"swagger": "2.0",
"info": {
"version": "1.0.0",
"title": "Swagger Petstore",
"description": "A sample API that uses a petstore as an example to demonstrate features in the swagger-2.0 specification",
"termsOfService": "http://swagger.io/terms/",
"contact": {
"name": "Swagger API Team"
},
"license": {
"name": "MIT"
}
},
"host": "petstore.swagger.io",
"basePath": "/api",
"schemes": [
"http"
],
"consumes": [
"application/json"
],
"produces": [
"application/json"
],
"paths": {
"/pets": {
"get": {
"description": "Returns all pets from the system that the user has access to",
"operationId": "findPets",
"produces": [
"application/json",
"application/xml",
"text/xml",
"text/html"
],
"parameters": [
{
"name": "tags",
"in": "query",
"description": "tags to filter by",
"required": false,
"type": "array",
"items": {
"type": "string"
},
"collectionFormat": "csv"
},
{
"name": "limit",
"in": "query",
"description": "maximum number of results to return",
"required": false,
"type": "integer",
"format": "int32"
}
],
"responses": {
"200": {
"description": "pet response",
"schema": {
"type": "array",
"items": {
"$ref": "#/definitions/Pet"
}
}
},
"default": {
"description": "unexpected error",
"schema": {
"$ref": "#/definitions/ErrorModel"
}
}
}
},
"post": {
"description": "Creates a new pet in the store. Duplicates are allowed",
"operationId": "addPet",
"produces": [
"application/json"
],
"parameters": [
{
"name": "pet",
"in": "body",
"description": "Pet to add to the store",
"required": true,
"schema": {
"$ref": "#/definitions/NewPet"
}
}
],
"responses": {
"200": {
"description": "pet response",
"schema": {
"$ref": "#/definitions/Pet"
}
},
"default": {
"description": "unexpected error",
"schema": {
"$ref": "#/definitions/ErrorModel"
}
}
}
}
},
"/pets/{id}": {
"get": {
"description": "Returns a user based on a single ID, if the user does not have access to the pet",
"operationId": "findPetById",
"produces": [
"application/json",
"application/xml",
"text/xml",
"text/html"
],
"parameters": [
{
"name": "id",
"in": "path",
"description": "ID of pet to fetch",
"required": true,
"type": "integer",
"format": "int64"
}
],
"responses": {
"200": {
"description": "pet response",
"schema": {
"$ref": "#/definitions/Pet"
}
},
"default": {
"description": "unexpected error",
"schema": {
"$ref": "#/definitions/ErrorModel"
}
}
}
},
"delete": {
"description": "deletes a single pet based on the ID supplied",
"operationId": "deletePet",
"parameters": [
{
"name": "id",
"in": "path",
"description": "ID of pet to delete",
"required": true,
"type": "integer",
"format": "int64"
}
],
"responses": {
"204": {
"description": "pet deleted"
},
"default": {
"description": "unexpected error",
"schema": {
"$ref": "#/definitions/ErrorModel"
}
}
}
}
}
},
"definitions": {
"Pet": {
"type": "object",
"allOf": [
{
"$ref": "#/definitions/NewPet"
},
{
"required": [
"id"
],
"properties": {
"id": {
"type": "integer",
"format": "int64"
}
}
}
]
},
"NewPet": {
"type": "object",
"required": [
"name"
],
"properties": {
"name": {
"type": "string"
},
"tag": {
"type": "string"
}
}
},
"ErrorModel": {
"type": "object",
"required": [
"code",
"message"
],
"properties": {
"code": {
"type": "integer",
"format": "int32"
},
"message": {
"type": "string"
}
}
}
}
} |
@dballance would you be able to run your perf test again using benchmark.js? Here is a how the performance test looks like using benchmark.js. You can play with $ node pefr.js const Benchmark = require('benchmark');
const { default: SwaggerClient } = require('../lib/index.js');
const spec = require('./data/huge.json');
const options = {
name: 'SwaggerClient.resolveSubtree',
defer: true,
// minSamples: 600,
expectedLodash: '1,588 ops/sec ±0.72% (674 runs sampled)',
expectedDeepmerge: '1,586 ops/sec ±0.91% (671 runs sampled)',
master: '1,576 ops/sec ±0.69% (674 runs sampled)',
async fn(deferred) {
await SwaggerClient.resolveSubtree(spec, []);
deferred.resolve();
},
};
module.exports = options;
// we're running as a script
if (module.parent === null) {
const bench = new Benchmark({
...options,
onComplete(event) {
console.info(String(event.target));
},
onError(event) {
console.error(event);
},
});
bench.run();
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Before we move forward, please have a look at my code review comments.
Update by running: npm i --package-lock-only According to https://docs.npmjs.com/cli/v8/configuring-npm/package-lock-json#lockfileversion lockfileVersion 2 is backwards compatible with 1 but adds extra metadata to aid with performance and reproducible builds. Additionally, swagger-ui already uses lockfileVersion 2 in its package-lock.json.
deep-extend does not support array merge. There was special code added to merge top-level arrays, but that was a shallow merge. Use deepmerge instead of deep-extend to merge arrays also. Default merge settings seem to work well - all tests pass. Extend all-of merge test case based on swagger-api/swagger-ui#7618 Fixes: 2f5bb86 ("Fix and test for swagger-ui swagger-api#3328: swagger-api/swagger-ui#3328. Added manual logic to merge arrays after calling deepExtend within `mergeDeep` function")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Let's merge and release.
## [3.18.1](v3.18.0...v3.18.1) (2022-01-14) ### Bug Fixes * **specmap:** fix deep merging when applying patch ([#2324](#2324)) ([65fcd22](65fcd22))
🎉 This PR is included in version 3.18.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
deep-extend does not support array merge. There was special code added
to merge top-level arrays, but that was a shallow merge.
Use deepmerge instead of deep-extend to merge arrays also. Default merge
settings seem to work well - all tests pass.
Add a test case based on swagger-api/swagger-ui#7618
Fixes: 2f5bb86 ("Fix and test for swagger-ui #3328: swagger-api/swagger-ui#3328. Added manual logic to merge arrays after calling deepExtend within
mergeDeep
function")@dballance do you mind checking this against your large spec? I wonder how well deepmerge does performance-wise and it would be nice to avoid extra complexity and just let the library do the merge. I would have tested it, but couldn't find your large spec in the tests. Reference #1150, #1190, #1209, #1217. Potential alternative if perfomance becomes an issue: https://www.npmjs.com/package/deepmerge-ts
Motivation and Context
Fixes swagger-api/swagger-ui#7618
Types of changes
package.json
)Checklist: