search

xogroup / felicity

Javascript object constructors and sample data based on Joi schema.
Other
111 stars 9 forks source link

Problem with alternatives.try when one of the alternatives is an object. #111

Closed einnjo closed 7 years ago

einnjo commented 7 years ago

Context

What are you trying to achieve or the steps to reproduce ?

Generate an example for a schema with alternatives, where one of the alternatives is an object.

const Felicity = require('felicity');
const Joi = require('joi');

const schema = Joi.alternatives().try(
    Joi.string(),
    Joi.object({ en: Joi.string(), es: Joi.string() })
);

Felicity.example(schema);

The error only shows up when the object schema is chosen as the alternative for the example.

What result did you expect ?

I expected a valid example for the object schema to be generated.

What result did you observe ?

image

WesTyler commented 7 years ago

Ah, yeah, I'm actually currently working on a big code-cleanup refactor that will fix this. The same issue exists on some array schemas as well... I was hoping to sneak in the fix before anyone got bitten by it. Going to bump this up in my priorities to try to get it out soon rather than later since you found it!

Thanks for checking out Felicity, and a huge thanks for filing the issue! :D :D :D

WesTyler commented 7 years ago

Also, because I feel the need to, I'd like to point out that I broke my "never use Joi ._inner" rule so that I could add in dynamic defaults faster. There's a PR open on Joi that, when merged, will mean I can remove the ._inner dependency completely.

This is why we have rules in place, haha. Sorry for breaking my own rule...

WesTyler commented 7 years ago

Okay, so I went ahead and pushed a quick patch via #b78e6e6, but it looks like Travis CI is having some issues. I'm going to give it 10-15 minutes before just deploying manually. Look for Felicity v1.3.2 within the hour.

WesTyler commented 7 years ago

Ok, manually published since Travis CI is down.

Let me know if you have any issues with v1.3.2 or above, and thanks again! :)

einnjo commented 7 years ago

@WesTyler that was fast, thank you.

I think I've run into an edge case:

My original example now works perfectly, but if I add the lowercase() option to the string schema I get: image This only happens inside alternatives, lowercase() works fine on a lone string and as part of an object.

WesTyler commented 7 years ago

Haha yes! I'm sorry to celebrate what is probably very frustrating to you, but I love when people find bugs in here. It's hard to think of every use case for the many Joi features.

Reopening this. I can't promise a fix as quickly this time; I want to dig a bit deeper to make sure I'm actually addressing the base problem this time.

WesTyler commented 7 years ago

Also, it looks like my experiment to re-write the entire example generation codebase is working out so far. I'll add this as an additional test, and hopefully the re-write will fix it and be done soon.

I'll also open up an issue for the re-write and link it here shortly for transparency.

einnjo commented 7 years ago

Don't worry, I appreciate what you are doing, felicity has been very helpful in testing my own codebase.

WesTyler commented 7 years ago

Glad to hear it has been helpful! :D

Larger issue that should (hopefully) resolve this, as promised: #112

WesTyler commented 7 years ago

Hey there, v2 is published with a major rewrite of the example data generation. I've specifically added some tests around the case like your most recent .lowercase() bug, and it looks to be working as expected now.

Check out v2, and please let me know if you run into any new edge cases or weird scenarios!