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.

Sign up for GitHub

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

Jump to bottom

if there is no Validator associated with a schema Field it generates illegal json #50

Closed
yanfali opened this issue Nov 29, 2016 · 19 comments
Closed

if there is no Validator associated with a schema Field it generates illegal json #50

yanfali opened this issue Nov 29, 2016 · 19 comments

Comments

@yanfali
Copy link
Contributor

yanfali commented Nov 29, 2016

e.g. "authentication": schema.Field{
Description: "Ignored authentication token",
Required: false,
},

Is perfectly valid go code, and the validator runs without complaint, but for the JSON schema it generates illegal JSON Schema. Should we make this a run time error if we discover it in the JSONSchema or in schema?
@rs
Copy link
Owner

rs commented Nov 29, 2016

Why is it invalid?

@yanfali
Copy link
Contributor Author

yanfali commented Nov 29, 2016 via email

@rs
Copy link
Owner

rs commented Nov 29, 2016

Wouldn't it be useful to let a field be of any type/format?

@yanfali
Copy link
Contributor Author

yanfali commented Nov 29, 2016 via email

@rs
Copy link
Owner

rs commented Nov 29, 2016

No particular use-case. We should certainly throw an error, you're right.

@smyrman
Copy link
Collaborator

smyrman commented Nov 29, 2016

It's perfectly legal to have a field of an arbitrary type in JSON Schema - even an empty dictionary is a valid JSON Schema!

@smyrman
Copy link
Collaborator

smyrman commented Nov 29, 2016

I would recommemd dealing with the comma situation, as the way we add commas feels quite hard-coded and as this issue proves, perhaps a bit error prone. Would it be possible to have anorther look at what the JSON package is doing? Or perhaps even utilize the JSON package for tag serialization?

One usecase I see for not specifying a validation for one particular field in a Schema, would be a logging API, or perhaps a job queue with arbitrary input parameters.

@smyrman
Copy link
Collaborator

smyrman commented Nov 29, 2016

Thinking out loud, perhaps one way could be to make it the responsibility of the "writer" (e.g. call it objectEncoder, arrayEncoder) to know when an object start, printe the leading/trailing { or [ and the commas between key/value pairs.

@yanfali
Copy link
Contributor Author

yanfali commented Nov 29, 2016

I think fixing the bug first would be good, then if you feel strongly enough to rewrite the serializer then open a separate issue. While it may be legal to generate a typeless schema I'm not really sure what that purpose it would serve other than to add to confusion.

@yanfali
Copy link
Contributor Author

yanfali commented Nov 29, 2016

My initial point is that using schema without a validator appears to be an incorrect usage of the API and should probably be considered an error by something like the JSONSchema. If you want the validator to be a wildcard wouldn't something like AnyOf be a better choice?

@rs do you consider a missing validator as an incorrect API usage or a valid use case?

@rs
Copy link
Owner

rs commented Nov 29, 2016

Yes in the current state of the code, we should not allow validator-less fields. It is easier to reopen that later if we feel like implementing this use-case.

@yanfali
Copy link
Contributor Author

yanfali commented Nov 29, 2016 via email

@smyrman
Copy link
Collaborator

smyrman commented Dec 1, 2016 •
edited

If you are going to raise an error with a nill validatior, please consider to at least make it a NotYetImplemented type error...

In my opinion, the use case seams completely valid. Even more so if nill validatiors are already allowed by the rest of the rest-layer code. @rs, is this the case?

To go more in depth with the use-case, consider a job queue API with:

Objekt task:

  • priority (int)
  • function (string)
  • parameters (any valid JSON)

Solving it with AnyOf would then require quite ugly code...

Cheers

@rs
Copy link
Owner

rs commented Dec 1, 2016

I'm not sure the current code expect a field with not validator.

@yanfali
Copy link
Contributor Author

yanfali commented Dec 1, 2016 via email

@smyrman
Copy link
Collaborator

smyrman commented Dec 1, 2016

Idomatic Go programs actually has a good history for making the nil value useful. Anyway, let's discuss this in a separate issue as you suggest.

@smyrman smyrman mentioned this issue Dec 1, 2016
Closed
yanfali pushed a commit to awakesecurity/rest-layer that referenced this issue Dec 10, 2016
Issue rs#50 prevent emitting invalid JSON
517a91a 
When a validator hasn't been set the JSONSchema emits invalid JSON.

 - add a test case replicating the issue
 - extend errWriter to track properties written
 - add a Comma helper which emits commas when at least 1 property has
 been written
 - extract function from encoding loop serializeField
 - change validator check to test validator for nil instead of just
 errors
    - errWriter takes care of errors
yanfali pushed a commit to awakesecurity/rest-layer that referenced this issue Dec 10, 2016
Issue rs#50 - detect improperly constructed Fields
570e045 
 - if both the Validator and the Schema have not been set, consider this
 an error and report it during validation.
yanfali pushed a commit to awakesecurity/rest-layer that referenced this issue Dec 10, 2016
Issue rs#50 prevent emitting invalid JSON
9b2abf0 
When a validator hasn't been set the JSONSchema emits invalid JSON.

 - add a test case replicating the issue
 - extend errWriter to track properties written
 - add a Comma helper which emits commas when at least 1 property has
 been written
 - extract function from encoding loop serializeField
 - change validator check to test validator for nil instead of just
 errors
    - errWriter takes care of errors
yanfali pushed a commit to awakesecurity/rest-layer that referenced this issue Dec 10, 2016
Issue rs#50 prevent emitting invalid JSON
1145a32 
When a validator hasn't been set the JSONSchema emits invalid JSON.

 - add a test case replicating the issue
 - extend errWriter to track properties written
 - add a Comma helper which emits commas when at least 1 property has
 been written
 - extract function from encoding loop serializeField
 - change validator check to test validator for nil instead of just
 errors
    - errWriter takes care of errors
rs pushed a commit that referenced this issue Dec 12, 2016
@yanfali @rs
Issue #50 fix up JSONSchema to not emit invalid JSON (#56)
eb49f3b 
When a validator hasn't been set the JSONSchema emits invalid JSON.

- Add a test case replicating the issue
- Create `fieldWriter` type which embeds `errWriter` as field
 tracking behavior is orthogaonal to `errWriters` purpose
- Extract function from encoding loop `serializeField`
- Change validator check to test validator for nil instead of just
 errors
    - ErrWriter takes care of errors
@smyrman
Copy link
Collaborator

smyrman commented Dec 13, 2016

@yanfali, is this now fixed?

@yanfali
Copy link
Contributor Author

yanfali commented Dec 13, 2016

Should be, there's a test that confirms the issue in the PR.

@smyrman
Copy link
Collaborator

smyrman commented Dec 13, 2016

Let's close it then:-)

@rs rs closed this as completed Dec 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rs @smyrman @yanfali