kristianmandrup / schema-to-yup

Schema to Yup validation
Other
282 stars 50 forks source link

Error complex json #20

Closed jmlopezo closed 5 years ago

jmlopezo commented 5 years ago

It does not generate validation for complex types, it only generates validation for first level objects. capture

kristianmandrup commented 5 years ago

Yup, I guess so... Haven't gotten this far yet, work in progress. Please help control if interested. I'm also working on a similar mapper to ElasticSearch schema where I've gotten a bit further and better architecture. Learning process how to do it the best way.

MarkRobbo commented 5 years ago

This is a very common use case which should probably be supported - currently testing JSON which has a nested structure using the generated yup object is impossible.

kristianmandrup commented 5 years ago

Hi @MarkRobbo, you're complete right. This should definitely be supported.

I've been working on a complete refactoring in the mono-repo branch

@mbrowne is also helping out here

You are most welcome to contribute in this effort. Even if you could just add some test cases for nested/complex JSON schemas, objects and expected results, that would be very helpful.

I expect to have more time to work on this the next 2 weeks. Have been out with severe back injury for a while... almost recovered now.

MarkRobbo commented 5 years ago

@kristianmandrup Get well soon, all the best.

I can certainly look into it. In the meantime, here is a very simple test case which currently does not work:

{
  "$id": "http://example.com/person",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "title": "Person",
  "type": "object",
  "properties": {
    "names": {
        "type": "object",
        "properties": {
          "firstName": {
            "type": "string",
            "pattern": "^[A-Za-z]*$"
          },
          "lastName": {
            "type": "string",
            "pattern": "^[A-Za-z]*$"
          }
        }
      }
    }
  }
}

I've only had a brief look through but it seems like the recursive parsing of objects is not working as you end up with an empty fields element in the yup schema. I believe otherwise most of the current code should be sufficient to parse the nested objects and place them in the correct position in the schema.

kristianmandrup commented 5 years ago

If you look at the mono-repo branch, you should be able to find a resolver folder or package with the logic to resolve:

Should include test code as well. You could try to port that into master branch for now and send a PR when working. Note that you will likely have to send the schema root along in the config object. See the api package build or builder folder for some hints.

Cheers!

MarkRobbo commented 5 years ago

Thanks for the pointers, that's pretty much exactly what I am looking for in terms of functionality. Do you have a vague timescale in terms of having the changes from the mono-repo branch published?

I usually would not want to interrupt a long running branch such as this especially if the feature has already been added.

kristianmandrup commented 5 years ago

Should be possible to finish this refactoring within the next 2-3 weeks. Please go ahead and make a quick fix though. Thanks.

kristianmandrup commented 5 years ago

You can look at our discussions here: #21

mbrowne commented 5 years ago

I'm unfortunately not going to be as available to work on this as I originally expected...I'm currently working on another project. I was also finding it challenging to design the initial implementation of the new version and think it might be better to wait for @kristianmandrup to complete more of the groundwork first.

One thing I didn't quite realize when I first started looking into this project is that the new version will be a lot more than just a JSON schema-to-yup converter and designed more as a generic solution to convert any input schema to any output schema via various packages that act as adapters...which is great, but obviously more complex.

kristianmandrup commented 5 years ago

Well, you are welcome to port the basic resolvers (ref and object) to the master branch for now. Then I'll work on the mono-repo generic branch over the next few weeks.

kristianmandrup commented 5 years ago

I can see I need a separate project that can convert from one schema form to another, in order to have a canonical schema form that can be traversed and processed. The traversed API should be another separate library.

I just encountered Avro Schema, used by Kafka. A mix between JSON schema and Avro seems to be a good fit for this canonical model. Avro had more rich types but miss some essential JSON schema features.

Let me know your thoughts. Could you extract your current walker logic to.a separate repo to work from?

MarkRobbo commented 5 years ago

I was looking at contributing to add at least object resolvers, which we very much need, but unfortunately I cannot get some of the basic tests to pass such as number range validation (toYupNumber.max.validate for instance) and am struggling to see why this is.

The value when running this.addConstraint always seems to end up as NaN when going through validateAndTransform, meaning 'no prop value' and no constraint being added. Is this something reproducible on master for you?

kristianmandrup commented 5 years ago

I will have a look at it tmrw. The latest on master branch might be a bit "buggy" since I ran into problems running Jest tests with the configuration. Jest tricks me up. I hate configuration hell.

If you can help or ensure Jest is set up correctly and document how to run it (ideally also individual tests) and much appreciate it!

kristianmandrup commented 5 years ago

Otherwise, go back to before the when condition was introduced (see git log). I got that working, but then encountered testing problems at some point, messed up some configuration I think.

MarkRobbo commented 5 years ago

I'm not convinced it's a test configuration issue as it seems fine to me in terms of running the tests simply by running jest. Running via patterns eg jest -t number also works fine.

It seems likely given the changes that the when condition has broken number min/max range validation, but I haven't had a chance to look into it very deeply.

MarkRobbo commented 5 years ago

@kristianmandrup any chance of getting master branch in a working condition anytime soon? Don't mind porting the object resolving myself but I would quite like a clean base to do it from as I'm not as familiar with the code base - and it would be very useful in an ongoing project

kristianmandrup commented 5 years ago

Hi Mark. I will be working on it the coming week. Have just been moving over to London, started new job, found a new flat, moved in, setting it up... Also had a hard time getting online on a stable connection unless at work where I have other commitments naturally. Sorry about that. Should be pretty easy to add the object resolve though...

MarkRobbo commented 5 years ago

Good luck in the new job - and thanks for the update! No need to apologise

kristianmandrup commented 5 years ago

Where are you located BTW?

kristianmandrup commented 5 years ago

Great thing is, I will likely be able to use this tooling at work, both on the front and backend so I can likely get some work done there as well ;)

mbrowne commented 5 years ago

@kristianmandrup While the new version is in development, do you think it might make sense for @MarkRobbo to work on a slightly older version of the master branch? I wonder if it builds successfully (or rather, works correctly) a couple commits back...just a thought.

kristianmandrup commented 5 years ago

Made some improvements to the master branch. It should now be able to handle complex schemas, ie. object with properties that are objects... However the code on master is "pretty bad"... IMO

kristianmandrup commented 5 years ago

Now finally works - includes tests to demonstrate