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

Travel all the errors in report #84

Merged
merged 1 commit into from Jun 8, 2017

Conversation

@edoput
Copy link
Collaborator

@edoput edoput commented Jun 7, 2017

An example is wort thousands words

{
	"type": "DeviceConfiguration",
	"general": {},
	"interfaces": [
		{
			"type": "wireless",
			"wireless": {
				"radio": "ath0",
				"mode": "access_point",
				"ssid": "test"
			},
			"addresses": [
				{
					"address": "192.168.1.20",
					"mask": 24,
					"family": "ipv4",
					"proto": "static"
				}
			]
		}
	]
}

Raise validation errors

netjsonconfig: JSON Schema violation
ValidationError {u'wireless': {u'radio': u'ath0', u'mode': u'access_point', u'ssid': u'test'}, u'type': u'wireless', u'addresses': [{u'address': u'192.168.1.20', u'mask': 24, u'family': u'ipv4', u'proto': u'static'}]} is not valid under any of the given schemas

Failed validating 'oneOf' in schema['properties']['interfaces']['items']:
    {'oneOf': [{'$ref': '#/definitions/network_interface'},
               {'$ref': '#/definitions/wireless_interface'},
               {'$ref': '#/definitions/bridge_interface'}],
     'title': 'Interface'}

On instance['interfaces'][0]:
    {u'addresses': [{u'address': u'192.168.1.20',
                     u'family': u'ipv4',
                     u'mask': 24,
                     u'proto': u'static'}],
     u'type': u'wireless',
     u'wireless': {u'mode': u'access_point',
                   u'radio': u'ath0',
                   u'ssid': u'test'}}

Against schema {'$ref': '#/definitions/network_interface'}
u'wireless' is not one of ['ethernet', 'virtual', 'loopback', 'other']

Against schema {'$ref': '#/definitions/wireless_interface'}
'name' is a required property

Against schema {'$ref': '#/definitions/bridge_interface'}
'name' is a required property

@edoput edoput force-pushed the edoput:i-will-find-your-errors branch from d0a5930 to bf64db8 Jun 7, 2017
@coveralls
Copy link

@coveralls coveralls commented Jun 7, 2017

Coverage Status

Coverage decreased (-0.1%) to 99.874% when pulling bf64db8 on EdoPut:i-will-find-your-errors into 8c26cd6 on openwisp:master.

@nemesisdesign
Copy link
Member

@nemesisdesign nemesisdesign commented Jun 7, 2017

@edoput, you mean the second block is the expected output that would be introduced by your change?

@coveralls
Copy link

@coveralls coveralls commented Jun 7, 2017

Coverage Status

Coverage decreased (-0.4%) to 99.625% when pulling 95f5d9b on EdoPut:i-will-find-your-errors into 8c26cd6 on openwisp:master.

@edoput
Copy link
Collaborator Author

@edoput edoput commented Jun 7, 2017

Yes the error reporting was working only on the first layer but the previous change introduces a tour into the second layer

But now I have a better one

Given the tree structure of the schema we can iterate over every error and subschema recursively and be able to print what is the problem in every definition, this is very verbose so it might not be useful for input with lots of schema errors.

ValidationError {u'wireless': {u'radio': u'ath0', u'ssid': u'test'}, u'type': u'wireless', u'name': u'wlan0', u'addresses': [{u'address': u'192.168.1.20', u'mask': 24, u'family': u'ipv4', u'proto': u'static'}]} is not valid under any of the given schemas

Failed validating 'oneOf' in schema['properties']['interfaces']['items']:
    {'oneOf': [{'$ref': '#/definitions/network_interface'},
               {'$ref': '#/definitions/wireless_interface'},
               {'$ref': '#/definitions/bridge_interface'}],
     'title': 'Interface'}

On instance['interfaces'][0]:
    {u'addresses': [{u'address': u'192.168.1.20',
                     u'family': u'ipv4',
                     u'mask': 24,
                     u'proto': u'static'}],
     u'name': u'wlan0',
     u'type': u'wireless',
     u'wireless': {u'radio': u'ath0', u'ssid': u'test'}}

Against schema {'$ref': '#/definitions/network_interface'}
u'wireless' is not one of ['ethernet', 'virtual', 'loopback', 'other']

Against schema {'$ref': '#/definitions/wireless_interface'}
{u'radio': u'ath0', u'ssid': u'test'} is not valid under any of the given schemas

Against schema {'$ref': '#/definitions/ap_wireless_settings'}
'mode' is a required property

Against schema {'$ref': '#/definitions/sta_wireless_settings'}
'mode' is a required property

Against schema {'$ref': '#/definitions/adhoc_wireless_settings'}
'bssid' is a required property

Against schema {'$ref': '#/definitions/monitor_wireless_settings'}
'mode' is a required property

Against schema {'$ref': '#/definitions/mesh_wireless_settings'}
'bssid' is a required property

Against schema {'$ref': '#/definitions/bridge_interface'}
'bridge_members' is a required property

I may have to see if this is a bettter fit for the job but I like where this is going so I call it complete

@nemesisdesign
Copy link
Member

@nemesisdesign nemesisdesign commented Jun 7, 2017

Can't wait for it, I also dislike the current situation with validation errors. It's also problematic to represent those errors visually in a UI, but that's another story.

@edoput
Copy link
Collaborator Author

@edoput edoput commented Jun 7, 2017

Ok as the ErrorTree from jsonschema does not recursively explore the errors I call it done.

Let's discuss the visual part and then merge if possible

We can have a tab \t for every nested level but it would be better to have something like the tree cli.

As an example here is the directory structure of netjsonconfig printed by tree -d .

netjsonconfig
├── backends
│   ├── airos
│   ├── base
│   ├── openvpn
│   │   ├── __pycache__
│   │   └── templates
│   │       └── __pycache__
│   ├── openwisp
│   │   ├── __pycache__
│   │   └── templates
│   ├── openwrt
│   │   ├── __pycache__
│   │   └── templates
│   └── __pycache__
└── __pycache__

The resulting visual aid should be something like this

On instance['interfaces'][0]:
    {u'addresses': [{u'address': u'192.168.1.20',
                     u'family': u'ipv4',
                     u'mask': 24,
                     u'proto': u'static'}],
     u'name': u'wlan0',
     u'type': u'wireless',
     u'wireless': {u'radio': u'ath0', u'ssid': u'test'}}

Against schema {'$ref': '#/definitions/network_interface'}
u'wireless' is not one of ['ethernet', 'virtual', 'loopback', 'other']

Against schema {'$ref': '#/definitions/wireless_interface'}
{u'radio': u'ath0', u'ssid': u'test'} is not valid under any of the given schemas
|
├── Against schema {'$ref': '#/definitions/ap_wireless_settings'}
|      'mode' is a required property
├── Against schema {'$ref': '#/definitions/sta_wireless_settings'}
|     'mode' is a required property
├── Against schema {'$ref': '#/definitions/adhoc_wireless_settings'}
 |     'bssid' is a required property
├── Against schema {'$ref': '#/definitions/monitor_wireless_settings'}
|      'mode' is a required property
└── Against schema {'$ref': '#/definitions/mesh_wireless_settings'}
     'bssid' is a required property

Against schema {'$ref': '#/definitions/bridge_interface'}
'bridge_members' is a required property

Or maybe I can experiment with colors but that may be another PR

@nemesisdesign
Copy link
Member

@nemesisdesign nemesisdesign commented Jun 7, 2017

@edoput great! Pay attention to the test coverage which is decreasing, it seems the new lines are not being executed during tests

@coveralls
Copy link

@coveralls coveralls commented Jun 8, 2017

Coverage Status

Coverage decreased (-0.1%) to 99.875% when pulling fc9b94f on EdoPut:i-will-find-your-errors into 8c26cd6 on openwisp:master.

@coveralls
Copy link

@coveralls coveralls commented Jun 8, 2017

Coverage Status

Coverage decreased (-0.1%) to 99.875% when pulling f3297cd on EdoPut:i-will-find-your-errors into 8c26cd6 on openwisp:master.

@edoput edoput force-pushed the edoput:i-will-find-your-errors branch from f3297cd to 325b511 Jun 8, 2017
@coveralls
Copy link

@coveralls coveralls commented Jun 8, 2017

Coverage Status

Coverage decreased (-0.1%) to 99.875% when pulling 325b511 on EdoPut:i-will-find-your-errors into 8c26cd6 on openwisp:master.

@nemesisdesign
Copy link
Member

@nemesisdesign nemesisdesign commented Jun 8, 2017

@edoput Great work, thanks for including the tests

@nemesisdesign nemesisdesign merged commit 6d95dd6 into openwisp:master Jun 8, 2017
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.1%) to 99.875%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@edoput edoput deleted the edoput:i-will-find-your-errors branch Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.