Fixes #19 - Json schemafor metadata #276

luan-cestari · 2015-01-09T14:29:27Z

Fixes #19

Created couple of methods and refactored other to support and enahnce the Metadata and related classes. Also created tests to support these changes.

jewzaam · 2015-01-09T14:44:04Z

metadata/src/main/java/com/redhat/lightblue/metadata/FieldConstraint.java

+    /**
+     * Returns the description of the FieldConstraint (for documentation propose)
+     */
+    String describeConstraint();


This name was confusing when I first saw it. Maybe if it were getDescription() it would be clearer?

I will change it. I used 'to describe' (the verb) intended to say that could be dynamic generated (using interpolation of different things to compose a meaning description) .

jewzaam · 2015-01-12T14:48:14Z

Have only gotten part way through reviewing this. Hopefully I can continue this afternoon but if not someone else is welcome to take it.

jewzaam · 2015-01-12T20:41:01Z

test/src/test/java/com/redhat/lightblue/test/metadata/FakeMetadataTest.java

+
+        JsonNode jsonSchema = metadata.getJSONSchema(entityName, version1);
+        String actual = jsonSchema.toString();
+        String expected = "{\"$schema\":\"http://json-schema.org/draft-04/schema#\",\"type\":\"object\",\"description\":\"JSON schema for entity 'user' version '1.0.0'\",\"properties\":{\"uid\":{\"type\":\"uid\"},\"nonrequid\":{\"type\":\"uid\",\"required\":\"Field not required constraint\"},\"iduid\":{\"type\":\"uid\",\"identity\":\"Field is part of identity constraint\"},\"personalInfo\":{\"type\":\"object\",\"lastName\":{\"type\":\"string\"},\"nonrequid\":{\"type\":\"uid\",\"required\":\"Field not required constraint\"},\"greeting\":{\"type\":\"string\"},\"department\":{\"type\":\"string\"},\"locale\":{\"type\":\"string\"},\"suffix\":{\"type\":\"string\"},\"title\":{\"type\":\"string\"},\"timezone\":{\"type\":\"string\"},\"faxNumber\":{\"type\":\"string\"},\"phoneNumber\":{\"type\":\"string\",\"matches\":\"^\\\\d{3}\\\\-\\\\d{4}\\\\ \\\\d{4}$\"},\"email\":{\"type\":\"string\"},\"company\":{\"type\":\"string\"},\"firstName\":{\"type\":\"string\"},\"requid\":{\"type\":\"uid\",\"required\":\"Field required constraint\"},\"emailConfirmed\":{\"type\":\"boolean\"}},\"updatedDate\":{\"type\":\"date\"},\"password\":{\"type\":\"object\",\"hashed\":{\"type\":\"string\"},\"salt\":{\"type\":\"string\"}},\"contactPermissions\":{\"type\":\"object\",\"allowMailContact\":{\"type\":\"boolean\"},\"allowThirdPartyContact\":{\"type\":\"boolean\"},\"allowPhoneContact\":{\"type\":\"boolean\"},\"allowFaxContact\":{\"type\":\"boolean\"},\"allowEmailContact\":{\"type\":\"boolean\"}},\"_id\":{\"type\":\"string\",\"identity\":\"Field is part of identity constraint\"},\"active\":{\"type\":\"boolean\"},\"login\":{\"type\":\"string\",\"maxLength\":\"64\",\"minLength\":\"1\",\"required\":\"Field required constraint\"},\"objectType\":{\"type\":\"string\"},\"requid\":{\"type\":\"uid\",\"description\":\"test\",\"required\":\"Field required constraint\"},\"createdDate\":{\"type\":\"date\"},\"required\":[\"login\",\"personalInfo.requid\",\"requid\"]}}";


Would be easier to manage in a test resource and loaded from disk. Using skyscreamer we can easily verify without worrying about formatting, meaning the file can be pretty printed.

jewzaam · 2015-01-12T20:45:54Z

Moving to in progress, please comment when updates are ready for review and I'll work to look as soon as I can.

jewzaam · 2015-01-19T15:06:01Z

@luan-cestari this needs updated still

Created a simple json-schema for validation and also moved most of the JSON content to files.

luan-cestari · 2015-01-21T16:40:18Z

updated the PR. @jewzaam

jewzaam · 2015-01-21T17:38:22Z

Just noticed FakeMetadataTest is in the test module. The unit tests for these changes should be in the metadata module. This test is to test the FakeMetadata class.

Generated schema is taking constraints on metadata fields and converting them to properties on the simple objects. For example, this:

      "iduid": {
        "type": "uid",
        "constraints": {
          "identity": true
        }
      },

becomes

        "iduid": {
            "type": "uid",
            "identity": "Field is part of identity constraint"
        },

It shouldn't do this. Identity isn't part of the json schema spec. I expected the json schema validation to catch this, but I guess it ignores additional properties. Identity should be treated the same as required. See comments below.

Also, required constraints should be reflected in a required field in the parent of the property in the json schema. They shouldn't be collected and reported at the root level property set. It's done today by using the lightblue full path, a period delimited string.

The "uid" type is converted directly to the json schema. There is no "uid" type in the spec. It should become a strong.

The "match" constraint is converted directly. It should be instead converted to a "pattern" attribute on the object representing the field.

I think min and max constraints are converted correctly.

I do not see a unit test including enum.

Moving back to in progress.

…data19

luan-cestari · 2015-01-28T21:18:30Z

Looking. The first change (about move to different module), I did but after I tested it I noticed that it can't be that way with those files, as the FakeMetadataTest class uses some classes from test module and test module have a dependency to the metadata module, so if I move the FakeMetadataTest class to metadata I would need the test module as dependency which would make a cyclic dependency issue.

About the other task to do, I'm working on it. I updated the json schema to have the validation necessary. Just one small question (to check if I got what you mean) one the snippets that you gave as example, so the first one* would be the output expected to be generated, right?

By 'first one' I mean:

      "iduid": {
        "type": "uid",
        "constraints": {
          "identity": true
        }
      },

jewzaam · 2015-01-29T14:28:42Z

If iduid was the only field in the metadata and lightblue uid type mapped to a json schema type of string without any regex:

    "properties": {
        "iduid": {
            "type": "string"
        }
    },
    "required": [
        "iduid"
    ]

coveralls · 2015-01-29T21:15:55Z

Coverage decreased (-1.0%) to 76.14% when pulling 9a6097a on luan-cestari:JSONSchemaforMetadata19 into cc6f4ae on lightblue-platform:master.

luan-cestari · 2015-01-30T18:44:08Z

Status summary:

metadata/test module change (result: not moved dua cyclic dependency)
Generated schema is taking constraints on metadata fields and converting them to properties (result: last commits fix that)
json schema validation to catch problems (result: create a simple schema. Do you want to use the 'schema.json' file from the metadata instead? https://github.com/lightblue-platform/lightblue-core/blob/cc6f4ae09bdfadb9372a7ea7e2e4476ad8accca0/metadata/src/main/resources/json-schema/metadata/schema.json )
required constraints should be reflected in a required (result: last commits fix that and also for identity)
The "uid" type should become a string. (result: last commits fix that )
"match" constraint be converted to a "pattern" attribute on the object representing the field.(result: last commits fix that )
min and max constraints are converted correctly.
unit test including enum.
Other things?

luan-cestari · 2015-01-30T18:45:36Z

@jewzaam I update the PR (also put a summary to help to track what have done and what we need to do) but I will focus on RDBMS integration test now, okay?

jewzaam · 2015-01-30T18:46:46Z

@luan-cestari thank you, yes please focus on rdbms. This can be dropped from Ana if necessary.

…data19

jewzaam · 2015-02-11T18:36:43Z

@luan-cestari given shift of focus and your work on non-rdbms issues could you consider closing out this issue?

Merged with the latest changes. Also, As asked in the PR, update the test to cover the enum type usage. I included also the enum definition in the schema generated as it seems strange if you have the schema with external references

luan-cestari · 2015-02-11T20:10:18Z

@jewzaam I just updated the PR to also cover the enum case. It was working fine, I just made some changes to also output the enum definition on the json as it is defined on teh entityinfo section which would not be included . The output would be something like https://gist.github.com/luan-cestari/a0ee0b58e9a08467d9a2 . Ready for review

luan-cestari · 2015-02-11T20:13:39Z

By the way , com.redhat.lightblue.metadata.EnumValue seems to always have empty/null description, is this fiend required at all? Could it be exemplified in the documentation http://docs.lightblue.io/language_specification/metadata.html ?

jewzaam · 2015-02-11T20:54:36Z

Created lightblue-platform/lightblue-docs-specs#7 for the missing documentation. Regarding it being empty, are you supplying descriptions for enum values? If not, yes it should be empty.

luan-cestari · 2015-02-12T14:10:27Z

It only accept and array of Strings. It seems that the description is only used when it is bind with a field called "annotatedValues" instead "values". I will try to use both cases and make an exception on the schema generation for that each case, ok?

jewzaam · 2015-02-12T14:20:40Z

Let's discuss enum structure on lightblue-platform/lightblue-docs-specs#7 since it needs documented.

luan-cestari · 2015-02-12T14:22:12Z

Alright, I will wait the resolution of that discussion. Let me know the
changes that it is pending to be made on this PR.

On Thu Feb 12 2015 at 12:20:42 PM Naveen Malik [email protected]
wrote:

Let's discuss enum structure on lightblue-platform/lightblue-docs-specs#7
lightblue-platform/lightblue-docs-specs#7
since it needs documented.

—
Reply to this email directly or view it on GitHub
#276 (comment)
.

coveralls · 2015-02-12T16:04:07Z

Coverage decreased (-1.0%) to 75.72% when pulling 64ff032 on luan-cestari:JSONSchemaforMetadata19 into 5fc5a3a on lightblue-platform:master.

luan-cestari · 2015-02-20T15:48:32Z

I will make little updates to create the Rest service for this feature

coveralls · 2015-02-20T15:51:25Z

Coverage decreased (-1.01%) to 75.71% when pulling 4e3f6ba on luan-cestari:JSONSchemaforMetadata19 into 5fc5a3a on lightblue-platform:master.

jewzaam · 2015-02-25T18:02:16Z

metadata/src/main/java/com/redhat/lightblue/metadata/FieldConstraint.java

+    /**
+     * Returns the description of the FieldConstraint (for documentation propose)
+     */
+    String getDescription();


Why do we need documentation on the constraint? There is nothing in metadata to describe why we set a constraint other than what might be saved in the field's description. But that's part of the field, not the constraint.

The documentation you mean the getDescription method right? This method is just a way to use polymorphism so I can get the specific description of each constraint without casting or checking instanceof. The description is used to generate a user-friendly message in the json schema

Why does the constraint need a description? Each property in the json schema needs a description, yes, but those are given by the field in lightblue metadata. Constraints don't have a description in json schema.

Maybe I chose the wrong word for the method name, it basically helps to generate an user text for a constraint and it can vary a lot. After the iterations we had for the issue that this PR aism to solve, the regex constraint (which uses 'pattern') is the only one using it right now and on this case it use to describe the pattern used for that constraint (which can vary for each request)

It might need to be specific to each constraint. Numeric value constraints (minimum, maximum, minLength, etc) will be a number in json (no quotes) vs a pattern which will be a string (with quotes). I haven't reviewed how this is used yet to see. Interestingly ArrayElementIdConstraint returns a bunch of text, so I assume given your comment this is broken or it isn't used in a generic way.

It was used, by I just quickly reviewed it before we talk about it, but I think it is just the regex case now, but it was used for other cases as well (when I had a different concept of the schema json that we would generate), so I can refactor it to something different just for regex case now if you think it would be better

… need lightblue-platform/lightblue-core#276 to be merged

…data19

coveralls · 2015-04-20T15:55:48Z

Coverage decreased (-1.35%) to 75.49% when pulling 4bd6bfa on luan-cestari:JSONSchemaforMetadata19 into ae0a1b7 on lightblue-platform:master.

luan-cestari added 2 commits January 8, 2015 18:48

First changes to make JSON document of the entity fields

a6875c8

Fixes lightblue-platform#19

5fee113

Created couple of methods and refactored other to support and enahnce the Metadata and related classes. Also created tests to support these changes.

luan-cestari added the ready for review label Jan 9, 2015

jewzaam reviewed Jan 9, 2015
View reviewed changes

luan-cestari added this to the Ana milestone Jan 9, 2015

renamed to getDescription()

1a184e6

jewzaam reviewed Jan 12, 2015
View reviewed changes

jewzaam added in progress and removed ready for review labels Jan 12, 2015

derek63 assigned luan-cestari Jan 19, 2015

Updated PR

e78688f

Created a simple json-schema for validation and also moved most of the JSON content to files.

jewzaam added ready for review and removed in progress labels Jan 21, 2015

jewzaam added in progress and removed ready for review labels Jan 21, 2015

luan-cestari changed the title ~~Json schemafor metadata19~~ Json schemafor metadata Fixes #19 Jan 28, 2015

Merge remote-tracking branch 'upstream/master' into JSONSchemaforMeta…

955c32c

…data19

Fixed some changes required

9a6097a

Merge remote-tracking branch 'upstream/master' into JSONSchemaforMeta…

ca40555

…data19

Merged & new enum test

64ff032

Merged with the latest changes. Also, As asked in the PR, update the test to cover the enum type usage. I included also the enum definition in the schema generated as it seems strange if you have the schema with external references

luan-cestari added ready for review and removed in progress labels Feb 11, 2015

Created a new role for json schema service

4e3f6ba

luan-cestari mentioned this pull request Feb 20, 2015

Fixes core#19 - GET JSON Schema for Metadata lightblue-platform/lightblue-rest#96

Closed

jewzaam modified the milestones: Bill, Ana Feb 20, 2015

jewzaam reviewed Feb 25, 2015
View reviewed changes

luan-cestari changed the title ~~Json schemafor metadata Fixes #19~~ Fixes #19 - Json schemafor metadata Mar 19, 2015

luan-cestari added a commit to luan-cestari/lightblue-rest that referenced this pull request Mar 19, 2015

Updated with the latest changes in the rest repository, but itt still…

cfb4613

… need lightblue-platform/lightblue-core#276 to be merged

Merge remote-tracking branch 'upstream/master' into JSONSchemaforMeta…

4bd6bfa

…data19

bvulaj force-pushed the master branch from f88739a to 823e5b8 Compare September 6, 2016 18:14

dcrissman closed this Jun 28, 2017

dcrissman removed the ready for review label Jun 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #19 - Json schemafor metadata #276

Fixes #19 - Json schemafor metadata #276

luan-cestari commented Jan 9, 2015

jewzaam Jan 9, 2015

luan-cestari Jan 9, 2015

jewzaam commented Jan 12, 2015

jewzaam Jan 12, 2015

jewzaam commented Jan 12, 2015

jewzaam commented Jan 19, 2015

luan-cestari commented Jan 21, 2015

jewzaam commented Jan 21, 2015

luan-cestari commented Jan 28, 2015

jewzaam commented Jan 29, 2015

coveralls commented Jan 29, 2015

luan-cestari commented Jan 30, 2015

luan-cestari commented Jan 30, 2015

jewzaam commented Jan 30, 2015

jewzaam commented Feb 11, 2015

luan-cestari commented Feb 11, 2015

luan-cestari commented Feb 11, 2015

jewzaam commented Feb 11, 2015

luan-cestari commented Feb 12, 2015

jewzaam commented Feb 12, 2015

luan-cestari commented Feb 12, 2015

coveralls commented Feb 12, 2015

luan-cestari commented Feb 20, 2015

coveralls commented Feb 20, 2015

jewzaam Feb 25, 2015

luan-cestari Feb 25, 2015

jewzaam Feb 25, 2015

luan-cestari Feb 25, 2015

jewzaam Feb 25, 2015

luan-cestari Feb 25, 2015

coveralls commented Apr 20, 2015

Fixes #19 - Json schemafor metadata #276

Fixes #19 - Json schemafor metadata #276

Conversation

luan-cestari commented Jan 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jewzaam commented Jan 12, 2015

Choose a reason for hiding this comment

jewzaam commented Jan 12, 2015

jewzaam commented Jan 19, 2015

luan-cestari commented Jan 21, 2015

jewzaam commented Jan 21, 2015

luan-cestari commented Jan 28, 2015

jewzaam commented Jan 29, 2015

coveralls commented Jan 29, 2015

luan-cestari commented Jan 30, 2015

luan-cestari commented Jan 30, 2015

jewzaam commented Jan 30, 2015

jewzaam commented Feb 11, 2015

luan-cestari commented Feb 11, 2015

luan-cestari commented Feb 11, 2015

jewzaam commented Feb 11, 2015

luan-cestari commented Feb 12, 2015

jewzaam commented Feb 12, 2015

luan-cestari commented Feb 12, 2015

coveralls commented Feb 12, 2015

luan-cestari commented Feb 20, 2015

coveralls commented Feb 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 20, 2015