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.

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

feat: Allow JSON Schema Faker configuration in specification #1899

Merged
merged 2 commits into from Oct 21, 2021
Merged

feat: Allow JSON Schema Faker configuration in specification #1899

merged 2 commits into from Oct 21, 2021

Conversation

shrink
Copy link
Contributor

@shrink shrink commented Sep 27, 2021

Related to...

Summary

I, and others, would like to be able to configure the JSON Schema Faker behaviour for our mock server. JSON Schema Faker supports a variety of options, and the underlying Faker instance does too. I am specifically interested in the resolveJsonPath option because it'll allow me to create contextually accurate examples.

openapi: 3.1.0

x-json-schema-faker:
  locale: de
  min-items: 2
  max-items: 10
  resolve-json-path: true

Checklist

  • The basics
    • I tested these changes manually in my local or dev environment
  • Tests
    • Added or updated
    • N/A
  • Event Tracking
    • I added event tracking and followed the event tracking guidelines
    • N/A
  • Error Reporting
    • I reported errors and followed the error reporting guidelines
    • N/A

Additional context

I don't love the implementation -- should configuration options for a mock server live in the API specification? -- but it's what was suggested in the canonical ticket for this issue, so that's what I've done! I figured that if that was the wrong approach then there'd have been some feedback in #1647, but if this is a bad approach, I'm happy to adjust.

I explicitly named the member x-json-schema-faker instead of x-faker because this applies to JSON Schema Faker, not Faker, and there's no relationship between the configuration options of each -- that is to say, minItems has no meaning in the context of Faker. An argument could be made that json-schema-faker already uses x-faker so it would be more consistent to use x-faker but x-faker is used by JSON Schema Faker to communicate a Faker option, so I think the inconsistency in the member name is justified to represent the different purpose.

I chose not to do any sort of validation against the x-json-schema-faker options because JSON Schema Faker doesn't do any validation itself, it accepts arbitrary values into a registry and then pulls them as needed -- no failure on bad input. I think it's important that this feature can enable configuration of JSON Schema Faker even as it changes, so while it'd be ideal to validate options -- automatically against some sort of configuration exposed by JSON Schema Faker -- it's not possible to do reliably.

I chose not to adjust the specification request logic, so both getHttpOperationsFromSpec and configureExtensionsFromSpec will make a request. I... am conflicted, on the one hand, the second lookup is wasteful but on the other, I don't want to mess with getHttpOperationsFromSpec because it's used in quite a few places -- so on balance, I decided two lookups is better than changing core logic and tests... but if someone feels strongly that the second request isn't acceptable I can make some changes (as far as I know json-schema-ref-parser doesn't do any caching so it'd be the responsibility of Prism to cache?).

@shrink shrink requested review from a team and rainum and removed request for a team September 27, 2021 22:30
@philsturgeon
Copy link
Contributor

Wow thank you @shrink, for the great pull request and for taking the time to write up your reasoning.

@ryotrellim what do you think of the approach here?

I'm usually somewhat against configuration in the mock for many things, but letting the author of the spec define how to build things seems a little helpful.

I'm not sure it exactly covers #1139 because its a global setting for all endpoints, unchangeable by the caller to cover tricky situations like pagination, but it does seem handy.

@shrink
Copy link
Contributor Author

shrink commented Oct 1, 2021

@philsturgeon I agree. For context, I am designing a JSON API Resource Revisioning extension and need resolveJsonPath enabled in JSON Schema Faker in order to provide a good developer experience with the (to be) bundled OpenAPI Specification + Prism mocking workflow. My immediate need would be resolved by this Pull Request or my previous feat: Resolve values using JSON Path in examples.

Context is important for a Mock to be a great Mock: currently, Prism provides great mock data when sourcing from an explicit specification-defined example (correct shape and contextually correct values) but the dynamic mock data is only guaranteed to be correct in shape (sometimes random lipsum is contextually correct, but not always).

From a Prism product perspective ("...a best-in-class Mock Server...") I don't believe this change (exposing the JSON Schema Faker configuration in the specification) is right, but I do think there are big gains to be made towards the product's stated goal. A best-in-class Mock Server is a Mock Server that is indistinguishable from the real thing to a client, and so dynamic mock data that is contextually correct is important. Being able to use jsonPath in examples is probably the single highest impact + lowest-cost way to get closer to contextually correct data and hope to see that in the short term, but long term I'm sure there are even more helpful/interesting ways to achieve that goal.

So in summary, as a user with an immediate need to provide contextually correct dynamic mock data, I'd like to see this merged so I can use jsonPath in my examples. As a person thinking about Prism's stated purpose and Prism 5 years from now, this change (introducing x-json-schema-faker configuration into the specification) feels wrong and I'd rather see #1898 merged. I probably should have shared these thoughts in that Pull Request instead before jumping to create this one!

I've removed #1139 from the list of related tickets, good catch, thank you.

@rainum
Copy link
Contributor

rainum commented Oct 1, 2021

Sorry guys, I'll be absent for a couple of weeks. Assigning this PR to @philsturgeon to unblock @shrink.

@rainum rainum requested review from philsturgeon and removed request for rainum October 1, 2021 13:24
@ryotrellim
Copy link
Contributor

@philsturgeon I'm fine with this approach.

@ryotrellim ryotrellim removed the request for review from philsturgeon October 5, 2021 19:21
@EdVinyard EdVinyard requested a review from a user October 19, 2021 19:19
@ghost
Copy link

ghost commented Oct 20, 2021

Generally looks fine. Please rebase with master and check tests.

@EdVinyard
Copy link
Contributor

Generally looks fine. Please rebase with master and check tests.

@lukasz-kuzynski-11sigma can you point me to the test run you're concerned about? I thought I had seen some problem here before, but after rebasing now I think the build succeeded (https://app.circleci.com/pipelines/github/stoplightio/prism/8383/workflows/fcd13771-5db7-4459-bc87-10749ef7402f).

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants