search

openapi-contrib / openapi-schema-to-json-schema

Due to the OpenAPI v3.0 and JSON Schema discrepancy, you can use this JS library to convert OpenAPI Schema objects to proper JSON Schema.
https://www.npmjs.com/package/@openapi-contrib/openapi-schema-to-json-schema
MIT License
242 stars 20 forks source link

Not all unnecessary items are removed from "required" array #3

Closed ozonep closed 4 years ago

ozonep commented 4 years ago

Currently there's a bug in a package when removing unnecessary items from "required" array. When iterating over "required" array it is mutated that makes "for" loop non-reliable, it can skip some properties that should be deleted.

Instead of mutating original array, I create new one and push items that are needed.

Also, replaced deep-equal with fast-deep-equal. Works the same way but should be faster.

philsturgeon commented 4 years ago

Thank you for this. I'm not sure I understand the problem, and there is no change to the tests. Could you update the tests to make sure the feature works, and to make it clear what its doing? Double win :D

ozonep commented 4 years ago

Thank you for this. I'm not sure I understand the problem, and there is no change to the tests. Could you update the tests to make sure the feature works, and to make it clear what its doing? Double win :D

Yeah, I will have a look at the tests, but I'm not sure they need to be updated though :)

Just to clarify what's going on:

"cleanRequired" function literates over "required" array to check if item should be removed or not for every particular case. The problem: if item is removed from array, it is removed from the same array "for" loop is iterating over, so data is mutated. That messes up index, so if there are 2+ items in array - some items will be skipped and not deleted.

So instead of mutating original array I create new one where I push all items that should stay required for that case.

philsturgeon commented 4 years ago

If behviour is changing, tests should change. Otherwise there is no way of knowing if what you want to happen is happening.

ozonep commented 4 years ago

Tests added! Previous tests were wrong, they tested only by removing first element in array, so they always passed. New tests will fail on old code.

philsturgeon commented 4 years ago

Hey! I spaced before, now I understand. Can you resolve the conflict and I'll get this merged ASAP.

XVincentX commented 4 years ago

Closed and rebased in #10 — thanks @ozonep