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 contract inclusion #43

Merged
merged 10 commits into from
Sep 28, 2017
Merged

Fix contract inclusion #43

merged 10 commits into from
Sep 28, 2017

Conversation

dgmora
Copy link
Contributor

@dgmora dgmora commented Sep 27, 2017

No description provided.

@jayniz
Copy link
Owner

jayniz commented Sep 27, 2017

What was broken?

'date' => {
'type' => 'object',
'properties' => {
'milis' => { 'type' => 'number' }
Copy link
Owner

Choose a reason for hiding this comment

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

millis?

@@ -125,16 +125,10 @@ def schema_contains?(options)

# Check all publish types for a compatible consume type
publish_types.each do |publish_type|
compatible_consume_type_found = false
consume_types.each do |consume_type|
next unless publish_type == consume_type or
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The or here looked wrong.
If you have two references like {'$ref' => '#/definitions/post' } it doesn't necessarily mean it's correct: The definition can be wrong in one of both sides. You need to ensure the type is fine


unless compatible_consume_type_found
return _e(:ERR_MISSING_MULTI_PUBLISH_MULTI_CONSUME, location, publish_type)
matched = consume_types.any? do |consume_type|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The loop does the same, just looked a bit easier to understand

@@ -175,11 +169,11 @@ def schema_contains?(options)
end

if consume['type'] == 'array'
sorted_publish = publish['items'].sort
Copy link
Contributor Author

@dgmora dgmora Sep 27, 2017

Choose a reason for hiding this comment

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

I didn't understand what was going on here. To me items looks pretty much like a oneOf, you need to find at least one match. I had some issues with multitype arrays, that's why I looked into it

Copy link
Owner

Choose a reason for hiding this comment

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

You need to find one, but not more than one (otherwise it would be anyOf)

end
@errors.empty?

# Make sure required properties in consume are required in publish
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split the method, and added the required check here too. This makes this new spec pass, because before we just looked the required inside definitions

@@ -233,7 +240,7 @@ def _e(error, location, extra = nil)
#
def data_for_pointer(data_or_pointer, schema)
data = nil
if data_or_pointer['type']
if data_or_pointer['type'] || data_or_pointer['oneOf']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before we ignored the oneOfs. This caused errors with 'oneOf' => [ { "$ref" => "#/definitions/post"] } (added a test here). This is the way usually lacerda transforms references.

@@ -23,15 +29,16 @@
'multi_props'=> { 'type'=>'array', 'items'=> [ {
'oneOf' => [
{ 'type' => 'string' },
{ '$ref' => '#/definitions/post' } ]
{ '$ref' => '#/definitions/date' } ]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I wrote this test, I didn't notice that this was a post that could reference another post, kind of recursively. I've got some errors that were hard to track...so I just changed it for something a bit simpler.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 99.832% when pulling f1d646b on fix-contract-inclusion into dfc8b6f on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 99.832% when pulling 7a28521 on fix-contract-inclusion into dfc8b6f on master.

@jayniz jayniz assigned dgmora and unassigned jayniz Sep 28, 2017
* dont support items with an array of types

* spacing
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 99.663% when pulling a3c292e on fix-contract-inclusion into dfc8b6f on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0009%) to 99.831% when pulling 8827da4 on fix-contract-inclusion into dfc8b6f on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 99.833% when pulling 179d66d on fix-contract-inclusion into dfc8b6f on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 99.833% when pulling 3a4915a on fix-contract-inclusion into dfc8b6f on master.

@dgmora dgmora merged commit 1cb28ee into master Sep 28, 2017
@dgmora dgmora deleted the fix-contract-inclusion branch September 28, 2017 15:28
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 99.833% when pulling e166c5c on fix-contract-inclusion into dfc8b6f on master.

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.

None yet

3 participants