clarin-eric / VLO

Virtual Language Observatory
GNU General Public License v3.0
14 stars 6 forks source link

Schema validation of VloConfig.xml #124

Closed wowasa closed 6 years ago

wowasa commented 6 years ago

Since fieldnames are configurable now via VloConfig.xml (see #112 ) we need a validation of the VloConfig to assure that...

  1. ...only valid keys are used
  2. ...mandatory keys are set with a key-value pair

The issue necessitates a code review since most mandatory keys exists due to the fact that we create ImmutableSets (f.e. in eu.clarin.cmdi.vlo.wicket.panels.search.AdvancedSearchOptionsPanel) which don't allow null values. Here we have the options:

  1. a valid fieldame must be set and the key-value must be mandatory
  2. the null value can be replaced by a default value (either a valid fieldname or and a dummy value)
  3. the setting is not necessary and is only set if not null
twagoo commented 6 years ago

For a first attempt, make all fields mandatory.

twagoo commented 6 years ago

Order of parameters should be fixed. Order of field definitions shouldn't.

twagoo commented 6 years ago
wowasa commented 6 years ago

I just replaced yesterday's issue_124 branch by a new one which doesn't take the path to the schema as a function parameter but reads the schema by classloader by default from resources in vlo-commons. Therefore only a modification in VloConfigMarshaller. In my view this makes more sense since the schema shouldn't be modified by end-users

twagoo commented 6 years ago

I agree, the schema can be fixed. What we still need to define is the behaviour in case of a validation error - should this terminate the unmarshalling which effectively means no further processing will be possible as without VloConfig we cannot do much? I think it would be better to ljust og any errors at ERROR level but try to continue unmarshalling. This behaviour can be defined by implementing and injecting a ValidationEventHandler. As far as I know the default handler terminates on validation error.

wowasa commented 6 years ago

good idea! I'm going to modify it in this way

twagoo commented 6 years ago

merged into development (see #174)