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

Add adapter and cache config validation #3836

Merged
merged 10 commits into from Jun 9, 2017

Conversation

Projects
3 participants
@kroepke
Member

kroepke commented May 16, 2017

config classes can now optionally validate their settings, e.g.
checking for missing files and non-sensical settings to raise mistakes
during creation of instances

this also allows uniform checking for unique names for entities as well
as helping plugin authors to provide a better user experience (e.g.
checking that a backend HTTP service is reachable etc)

how much or how little is done depends on the plugin author

failed validation currently does not prevent saving the configuration,
but the UI should warn the user

also auto-generates the name property from the title unless it has been modified manually

#3831
#3835

@kroepke kroepke added this to the 2.3.0 milestone May 16, 2017

@kroepke kroepke requested a review from edmundoa May 16, 2017

@kroepke kroepke added the in progress label May 16, 2017

@kroepke

This comment has been minimized.

Member

kroepke commented May 16, 2017

This is for initial review before adding the same mechanism to lookup tables and caches.

@kroepke kroepke force-pushed the adapter-config-validation branch from 3ec566c to a3b2f97 May 17, 2017

@kroepke

This comment has been minimized.

Member

kroepke commented May 17, 2017

rebased to master

@edmundoa

Other than the in-line remarks, I like the validation 👍

@@ -76,7 +116,31 @@ const DataAdapterForm = React.createClass({
promise = LookupTableDataAdaptersActions.update(this.state.dataAdapter);
}
promise.then(() => { this.props.saved(); });
promise.then(() => {

This comment has been minimized.

@edmundoa

edmundoa May 17, 2017

Member

Shouldn't we enforce the state to be valid before persisting (or trying to persist) the data adapter?

This comment has been minimized.

@edmundoa

edmundoa May 17, 2017

Member

It also feels a bit odd when there was an exception (because the name is already taken) but you don't get any feedback that the operation failed. We should add a couple of UserNotification to avoid that.

This comment has been minimized.

@kroepke

kroepke May 17, 2017

Member

Indeed!

This comment has been minimized.

@kroepke

kroepke May 17, 2017

Member

Failed validation might include somthing like a missing file, which should not prevent the entity to be created, I think. Other structural things will be returned as an exception, which handling is missing yet, correct.

This comment has been minimized.

@edmundoa
let generateAdapterName = this.state.generateAdapterName;
if (generateAdapterName && event.target.name === 'title') {
// generate the name
dataAdapter.name = this._sanitizeTitle(dataAdapter.title);

This comment has been minimized.

@edmundoa

edmundoa May 17, 2017

Member

Should we also sanitize titles when the user edits them afterwards? By the way, why do we need two different names (title and name) to refer to the adapter? I guess one of them is for the pipelines, but I'm not really sure :-)

This comment has been minimized.

@kroepke

kroepke May 17, 2017

Member

The title is solely for display purposes, the name is for referring to them in pipelines (and other places), correct.

The initial sanitizing is basically just a nicer way of helping the user to fill in that field, I think once the user manually changes it, we should keep it. It also really is just a string and can be anything, the regex is just to make it a bit more readable.

This comment has been minimized.

@edmundoa

edmundoa May 17, 2017

Member

Ah, then it is fine as it is imo. We should mention that the name is meant to be used from other places like pipelines in the help text (just to clarify it).

@Path("adapters/validate")
@NoAuditEvent("Validation only")
@ApiOperation(value = "Validate the data adapter config")
public ValidationResult validateAdapter(@Valid @ApiParam DataAdapterApi toValidate) {

This comment has been minimized.

@edmundoa

edmundoa May 17, 2017

Member

Editing a data adapter and leaving an empty name does not highlight the field as not valid. The response from the server in this case is a 400 instead of 200. I guess it's because hibernate is validating the DataAdapterApi and it can't find a value for name.

I think we can live with this, as the form is in the end validated by the browser and catches that issue, but I also guess is a bit confusing when you don't know how the code underneath works.

This comment has been minimized.

@kroepke

kroepke May 17, 2017

Member

Yes, I'll see if we can make this a bit more obvious by listing the possible return codes.

This comment has been minimized.

@edmundoa

edmundoa May 17, 2017

Member

Yes, anyway as I said, in the end the browser stops you from submitting that form, so it's just a bit of inconsistency :-)

@kroepke kroepke added this to In Progress in Lookup Tables May 18, 2017

@kroepke kroepke changed the title from [WIP] add adapter config validation to Add adapter and cache config validation May 29, 2017

@kroepke kroepke requested a review from bernd May 29, 2017

@bernd bernd self-assigned this May 29, 2017

@bernd

This comment has been minimized.

Member

bernd commented May 29, 2017

I am running into some issues when a validation fails. The data I entered in other fields will disappear.

Cache

peek 2017-05-29 15-45

Data Adapter

peek 2017-05-29 15-48

@bernd

There is a state issue with the validations: #3836 (comment)

@bernd

This comment has been minimized.

Member

bernd commented Jun 7, 2017

The state issue does not show up when using this branch but only when rebasing against master. Something in master has changed that triggers this issue.

kroepke added some commits May 16, 2017

add adapter config validation
config classes can now optionally validate their settings, e.g.
checking for missing files and non-sensical settings to raise mistakes
during creation of instances

this also allows uniform checking for unique names for entities as well
as helping plugin authors to provide a better user experience (e.g.
checking that a backend HTTP service is reachable etc)

how much or how little is done depends on the plugin author

failed validation currently does not prevent saving the configuration,
but the UI should warn the user

also auto-generates the name property from the title unless it has been modified manually

#3831
#3835
add config validation to caches
declare proptypes consistently and turn off linter warnings for unused props

auto-generate name to caches from title, when name isn't set explicitly
add validation annotations
minor code layout tweak in jsx for csv file adapter
compare the correct subtree from props to avoid overwriting state
turn off errorstate fetching for create forms (which would re-render them all the time)

@kroepke kroepke force-pushed the adapter-config-validation branch from 643b9ae to 6b28cbd Jun 9, 2017

@bernd

bernd approved these changes Jun 9, 2017

@bernd bernd merged commit 46a6ff1 into master Jun 9, 2017

1 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
graylog-project/pr Jenkins build graylog-project-pr-snapshot 191 is running
Details
ci-web-linter Jenkins build graylog-pr-linter-check 1714 has succeeded
Details

@bernd bernd removed the in progress label Jun 9, 2017

@bernd bernd removed the ready-for-review label Jun 9, 2017

@bernd bernd deleted the adapter-config-validation branch Jun 9, 2017

@bernd bernd moved this from In Progress to Done in Lookup Tables Jun 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment