Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

Improve the configuration validation error messages #1675

Closed
chrisprice opened this issue May 12, 2020 · 6 comments · Fixed by #1688
Closed

Improve the configuration validation error messages #1675

chrisprice opened this issue May 12, 2020 · 6 comments · Fixed by #1688
Labels
improved-ux Issues related to improving the experience of the tool, the documentation or the project for users

Comments

@chrisprice
Copy link

Feature request

I've misconfigured one of my fields but it's not obvious to me which one it is from the error message.

java.lang.IllegalStateException: No data types with type  string
        at com.scottlogic.datahelix.generator.common.profile.StandardSpecificFieldType.lambda$from$1(StandardSpecificFieldType.java:63)
        at java.util.Optional.orElseThrow(Optional.java:290)
        at com.scottlogic.datahelix.generator.common.profile.StandardSpecificFieldType.from(StandardSpecificFieldType.java:63)
        at com.scottlogic.datahelix.generator.profile.services.FieldService.specificFieldTypeFromString(FieldService.java:51)
        at com.scottlogic.datahelix.generator.profile.validators.profile.ConstraintValidator.getFieldType(ConstraintValidator.java:145)

Ideally the above would give me not only the misconfigured field but also suggestions on what I need to do to fix it.

@chrisprice chrisprice added the enhancement Issues related to improving the codebase, the documentation or process within the project label May 12, 2020
@chrisprice
Copy link
Author

So the error was a leading space in the field type i.e. " string" rather than "string". Quoting the invalid value would also be really helpful in tracking this down.

@ghost ghost added improved-ux Issues related to improving the experience of the tool, the documentation or the project for users and removed enhancement Issues related to improving the codebase, the documentation or process within the project labels Jun 10, 2020
@tjohnson-scottlogic
Copy link
Contributor

Hi @chrisprice, just to let you know we’ve not forgotten about this, I’m hoping to address it over the next couple of weeks.

matthewdunsdon added a commit to matthewdunsdon/datahelix that referenced this issue Jul 9, 2020
Improve test generators / builders for DTOs and `GenerationConfigSource`
to simplify writing tests cases
@matthewdunsdon
Copy link
Contributor

Current errors (some similar checks not in table).

Number Outcome Message Notes
1 Should reject fields containing a null [ERROR] Unexpected exception type thrown ==> expected: <com.scottlogic.datahelix.generator.common.ValidationException> but was: <java.lang.NullPointerException> Should provide a helpful warning.
2 Should reject missing fields and constraints Fields must be specified
Constraints must be specified
-
3 Validate field with null type fails Field type must be specified | Field: test Might be good to point user to page which explains the field types.
4 Validate field with empty name fails Field name must be specified Might be difficult to find where this field is.
5 Validate field with null field fails Field must not be null Could recommend the null to be removed from the list of fields.
6 Validate field with null name fails Field name must be specified Could explain that field name shouldn't be null.
7 Validate field with null name and null type fails Field name must be specified
Field type must be specified | Field: Unnamed field
-
8 Validate constraint with null constraint fails Constraint must not be null Could recommend the null to be removed from the list of constraints.
9 Validate atomic constraint with undefined field fails unknown must be defined in fields | Field: unknown | Constraint: equalTo Field name should be quoted 'unknown' and seems unclear.
10 Validate atomic constraint with null field fails Field must be specified | Field: null | Constraint: equalTo Might be worth clarifying that the issue is with the 'field' property on the constraint.
11 Validate atomic constraint with invalid constraint fails Invalid json: { \"some\": \"json\" } Does not explain that this is an invalid constraint,
12 Validate atomic constraint with incorrect value type fails Value 1 must be a string | Field: text | Constraint: equalTo Might be a misleading instruction, as the value could be correct but the user may have specified the wrong field.
13 Validate atomic constraint with null value fails Values must be specified | Field: text | Constraint: equalTo Not clear what values is, which is meant to refer to the value property. Maybe we need an error which refers to the actual field name (such as 'equalTo').
14 Validate atomic constraint with empty field fails Field must be specified | Field: | Constraint: equalTo See entry 10, as well as quoting where appropriate.
15 Validate in map constraint with invalid data fails - No error currently shown if the types data is not correct.
16 Validate in map constraint with empty value fails Values must be specified | Constraint: inMap See entry 13
20 Validate in map constraint with null value fails Values must be specified | Constraint: inMap See entry 13
21 Validate relational constraint with null other field fails Related field must be specified | Constraint: greaterThanField See entry 10, though related field name might be worth specifying.
22 Validate relational constraint with empty other field fails Related field must be specified | Constraint: greaterThanField See entry 21
23 Validate relational constraint with granularity on unsupported field type fails - No warning when offset set without, even if offsetUnit not specified, on unsupported field type (such as strings)
24 Validate relational constraint with granularity unit on unsupported field type fails Granularity days is not supported for string fields | Constraint: afterField Might be worth quoting the granularity value.
27 Validate relational constraint with invalid field type for date field constraint fails - The afterField constraint is for date fields, so should fail when given a field of different type (such as numeric)
29 Validate relational constraint with invalid field type for numeric field constraint fails - The greaterThanField constraint is for numeric fields, so should fail when given a field of different type (such as date)
30 Validate relational constraint with field and other field of differing type fails Field type integer doesn't match related field type text | Constraint: greaterThanField Seems confusing
34 Validate value type with boolean type and string value fails - No warning when supplying string value for a boolean field for constraints such as equalTo.
38 Validate granularity constraint with empty value fails Granularity is not supported | error info The granularity value needs quoting. Maybe the constraint name should be specified.
39 Validate granularity constraint with invalid value fails Granularity mills is not supported | error info See entry 38
40 Validate granularity constraint with value type boolean fails Granularity true is not supported for boolean fields | Field: boolean | Constraint: granularTo Might be better to lead with the restriction on granularity type.
43 Validate granularity constraint with value type string fails Granularity test is not supported for string fields | Field: text | Constraint: granularTo See entry 40
46 Validate all of constraint with unspecified sub constraints fails Sub constraints must be specified | Constraint: allOf Might be worth providing a link to an example of the allOf constraint
47 Validate all of constraint with null sub constraint fails Constraint must not be null Could provide some context indicating this is a sub constraint of allOf.
48 Validate all of constraint with empty sub constraints fails Sub constraints must be specified | Constraint: allOf Could change to state that at least one sub constraint must be specified.
51 Validate conditional constraint with unspecified if constraint fails Constraint must not be null Could provide some context indicating this is the if part of the if constraint.
52 Validate conditional constraint with unspecified then constraint fails 'if' constraint must also have an associated 'then' constraint -
53 Validate conditional constraint with invalid else constraint fails - Need to add check for the else par of the if constraint is not being verified.
54 Validate not constraint with unspecified constraint fails Constraint must not be null Could provide some context indicating this is the sub constraint of the not constraint.
56 Validate any of constraint with unspecified sub constraints fails Sub constraints must be specified | Constraint: anyOf See entry 46
57 Validate any of constraint with null sub constraint fails Constraint must not be null See entry 47
59 Validate any of constraint with empty sub constraints fails Sub constraints must be specified | Constraint: anyOf See entry 48
66 Validate shorter than constraint with value less than min fails String length must have a value >= 0, currently is -1 Could include context showing that is is the shorterThan constraint.
67 Validate shorter than constraint with invalid field type fails Expected field type STRING doesn't match field type NUMERIC | Field: integer | Constraint: shorterThan Confusing message.
73 Validate in set constraint with null value fails In set values must be specified | Field: text | Constraint: inSet -
75 Validate in set constraint with empty value fails In set values must be specified | Field: text | Constraint: inSet -
78 Validate granular to constraint with value type boolean fails Granularity true is not supported for boolean fields | Field: boolean | Constraint: granularTo Confusing message.
80 Validate granular to constraint with value type string fails Granularity test is not supported for string fields | Field: text | Constraint: granularTo Confusing message.
83 Validate of length constraint with value less than min fails String length must have a value >= 0, currently is -1 See entry 66
84 Validate of length constraint with invalid field type fails Expected field type STRING doesn't match field type NUMERIC | Field: decimal | Constraint: ofLength Confusing message.
85 Validate of length constraint with value greater than max fails String length must have a value <= 1000, currently is 1001 See entry 66
89 Validate regex constraint with invalid field type fails Expected field type STRING doesn't match field type NUMERIC | Field: decimal | Constraint: containingRegex See entry 84
91 Validate regex constraint with invalid regex fails Regex is invalid | Dangling meta character '*' near index 2\n/*****/\n ^ -
94 Validate regex constraint with null regex fails Text must be specified | Field: text | Constraint: containingRegex -
95 Validate numeric constraint with null value fails Number must be specified | Field: integer | Constraint: lessThan Could include a reference to the specific field name that needs setting.
99 Validate numeric constraint with value less than min fails Number must have a value >= -100000000000000000000, currently is -100000000000000000001 See entry 66
100 Validate numeric constraint with invalid field type fails Expected field type NUMERIC doesn't match field type STRING | Field: text | Constraint: lessThan Confusing message.
101 Validate numeric constraint with value greater than max fails Number must have a value <= 100000000000000000000, currently is 100000000000000000001 See entry 66
104 Validate temporal constraint with unspecified datetime fails DateTime must be specified | Field: datetime | Constraint: after Could include a reference to the specific field name that needs setting.
105 Validate temporal constraint with datetime before min fails Dates must be between 0001-01-01T00:00:00.000Z and 9999-12-31T23:59:59.999Z | Field: datetime | Constraint: after -
106 Validate temporal constraint with null time fails Time must be specified | Field: time | Constraint: after Could include a reference to the specific field name that needs setting.
107 Validate temporal constraint with unspecified time fails Time must be specified | Field: time | Constraint: after See entry 106
108 Validate temporal constraint with null datetime fails DateTime must be specified | Field: datetime | Constraint: after See entry 106
110 Validate temporal constraint with invalid datetime fails Date string 'invalid datetime' must be in ISO-8601 format: Either yyyy-MM-ddTHH:mm:ss.SSS[Z] between 0001-01-01T00:00:00.000Z and 9999-12-31T23:59:59.999Z or yyyy-mm-dd between 0001-01-01 and 9999-12-31 | Field: datetime | Constraint: after -
111 Validate temporal constraint with datetime after max fails Dates must be between 0001-01-01T00:00:00.000Z and 9999-12-31T23:59:59.999Z | Field: datetime | Constraint: after See entry 105
112 Validate temporal constraint with invalid time fails Time string invalid time must be in ISO-8601 format: Either hh:mm:ss or hh:mm:ss.ms | Field: time | Constraint: after -
116 Validate longer than constraint with invalid field type fails Expected field type STRING doesn't match field type NUMERIC | Field: decimal | Constraint: longerThan Confusing message.
117 Validate longer than constraint with value too large fails String length must have a value <= 1000, currently is 1001 See entry 66
118 Validate profile with empty fields fails Fields must be specified Could provide a link to what fields are supported or a valid profile.
119 Validate profile with null constraints fails Constraints must be specified Could provide a link to what fields are supported or a valid profile.
120 Validate profile with null fields fails Fields must be specified Could provide a link to what fields are supported or a valid profile.
121 Validate profile with unique field referenced in if fails Unique field uniq cannot be referenced in IF statement Should quote field name.
122 Validate profile with null fields and null constraints fails Fields must be specified Could provide a link to what fields are supported or a valid profile.
123 Validate profile with unique fields in non minimal combination strategy fails Unique fields do not work when not using Minimal combination strategy -
124 Validate profile with non unique fields fails Field names must be unique | Duplicates: test -

@matthewdunsdon
Copy link
Contributor

Observations considering the validation / error messages:

  1. Not enough interpolated values in the message text (such as field names) are quoted.
  2. Seems like for many errors, where we know actually know the field / constraint type, there is no reason why we could not offer a link to some part of the documentation.
    • There are no predictable urls (or arguably no long-term stable url) for getting to the documentation of a particular field or constraint.
  3. There is an inconsistent application of constraint context
    • Some messages don't mention constraint name and / or field name when they could.
    • A few messages for grammatical constraints, could help provide more context.
  4. Validation checks are not as extensive as they could be.
    • See java.lang.IllegalStateException: No data types with type string issue
    • See entries in table with "-" as the message, which are validation cases which I believe are missing.
  5. Some messages could do with a rewording:
    • Expected field type STRING doesn't match field type NUMERIC | Field: decimal | Constraint: longerThan is trying to say something like The 'longerThan' constraint operates on fields that are of a STRING type, however the specified field named 'decimal' is of a NUMERIC type.
    • Values must be specified | Field: text | Constraint: equalTo is trying to say The 'equalTo' constraint for field 'text' needs to have a text value specified for the 'equalTo' property
    • Value 1 must be a string | Field: text | Constraint: equalTo is trying to say The 'equalTo' constraint for field 'text' needs to have a text value specified for the 'equalTo' property, but the numeric value 1 was specified. Either choose a numeric field or change the value to be a string

@matthewdunsdon
Copy link
Contributor

matthewdunsdon commented Jul 15, 2020

(Observation 1) Not enough interpolated values are quoted

I am comfortable picking this up as part of this issue.

(Observation 2) Messages offer a link to some part of the documentation

Propose that this gets looked at as part of a different issue (#1689).

  • Requires a documentation URL strategy (for getting to particular field or constraint documentation) to be defined first.
(Observation 3) Inconsistent context for finding constraint

Propose that this gets looked at as part of a different issue (#1690).

  • I acknowledge that for some error messages it could be difficult to identify what part of the profile could be the problem, but not sure how common these cases will be.
(Observation 4) Validation checks are not as extensive as they could be

I am comfortable picking this up as part of this issue.

(Observation 5) Some messages could do with a rewording

Propose that this gets looked at as part of a different issue.

  • Seems like research needed for defining what a good validation messages should be.
  • There are 70+ unique message which would need updating, so could be a big piece of work.

@tjohnson-scottlogic
Copy link
Contributor

Sounds good to me, very thorough bit of work!

matthewdunsdon added a commit to matthewdunsdon/datahelix that referenced this issue Jul 15, 2020
matthewdunsdon added a commit to matthewdunsdon/datahelix that referenced this issue Jul 15, 2020
... so that it is clearer for users to interpret.
matthewdunsdon added a commit to matthewdunsdon/datahelix that referenced this issue Jul 15, 2020
matthewdunsdon added a commit to matthewdunsdon/datahelix that referenced this issue Jul 15, 2020
matthewdunsdon added a commit to matthewdunsdon/datahelix that referenced this issue Jul 15, 2020
matthewdunsdon added a commit to matthewdunsdon/datahelix that referenced this issue Jul 17, 2020
matthewdunsdon added a commit to matthewdunsdon/datahelix that referenced this issue Jul 17, 2020
matthewdunsdon added a commit to matthewdunsdon/datahelix that referenced this issue Jul 17, 2020
matthewdunsdon added a commit to matthewdunsdon/datahelix that referenced this issue Jul 17, 2020
matthewdunsdon added a commit to matthewdunsdon/datahelix that referenced this issue Jul 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
improved-ux Issues related to improving the experience of the tool, the documentation or the project for users
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants