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

Enable polymorphism #30

Closed
wants to merge 25 commits into from
Closed

Conversation

nakamorichi
Copy link
Contributor

@nakamorichi nakamorichi commented Oct 25, 2016

...by merging members of "allOf" into a single properties object and a single requirements array.

Fixes #29

…rties object and a single requirements array.
@nakamorichi nakamorichi changed the title Enable polymorphism #29 Enable polymorphism Oct 25, 2016
@nakamorichi nakamorichi changed the title #29 Enable polymorphism Enable polymorphism Oct 25, 2016
@briananderson1222
Copy link
Collaborator

@Kitanotori Thanks for the contributions here. Are there any unit tests that you can add to ensure that this functionality doesn't undergo regression in the future?

@briananderson1222
Copy link
Collaborator

If not, I plan to go back and add some tests for the additionalProperties functionality I added and might be able to take a look at adding tests here. I think with those two change it may warrant a new release.

@nakamorichi
Copy link
Contributor Author

Unfortunately no time to write tests currently.

@celalo
Copy link

celalo commented Feb 4, 2017

This does not handle $ref only members of allOf, does it? Example:

"definitions": {
        "dealer_output": {
            "title": "Dealer Output",
            "allOf": [
                {
                    "$ref": "#/definitions/dealer_input"
                },
                {
                    "type": "object",
                    "properties": {
                        "_id": {
                            "type": "string",
                            "format": "uuid"
                        }
                    },
                    "required": [
                        "_id"
                    ]
                }
            ]
        }
.
.
.
.
.

@celalo
Copy link

celalo commented Feb 5, 2017

This is what I came up with to solve the problem I mentioned above: celalo@8c4370a

@briananderson1222
Copy link
Collaborator

I just saw this! Thanks for pushing forward on these updates! From what I can tell the Travis job is failing due to some nested dependencies that no longer support node 0.11. Feel free to update to 4+ in the travis.yml and see if we can get the tests to pass here.

@nakamorichi
Copy link
Contributor Author

@briananderson1222 I fixed Travis's and Mocha's settings (timeout limit was too low). Now only the test I added, containing example of allOf definition, fails.

Petri Kivikangas added 4 commits March 9, 2017 21:12
- date and date-time were not processed correctly when their type was correctly specified as ‘string’
- date, dateTime, password, double, float, long are not valid types
# Conflicts:
#	package.json
#	test/index.js
…g’s except ‘date’ and ‘date-time’ as String
if (!_.isEmpty(mergedReqs)) definition['required'] = Array.from(mergedReqs);
delete definition['allOf'];
}

var getSchema = function (objectName, fullObject) {
var props = {};
var required = fullObject.required || [];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 247 (var required = fullObject.required || [];) should be moved to line 251 (i.e. after processPolymorphic runs if the definition is polymorphic) otherwise we're not adding in the merged required fields.

sergef added a commit to sergef/swagger-mongoose that referenced this pull request Apr 19, 2018
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 this pull request may close these issues.

Support for polymorphism missing ("allOf")?
4 participants