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

fix: edge case where component would sometimes be marked a string #174

Merged
merged 1 commit into from
Apr 30, 2020

Conversation

erunion
Copy link
Member

@erunion erunion commented Apr 30, 2020

🎟 Asana

🆘 Problem

This resolves an edge case that was introduced in some logic I added to repair schemas without a type that was sometimes marking the schema object as a string. This would result in the following "broken" UI:

Screen Shot 2020-04-29 at 10 42 36 PM

You can see it in action here: http://bin.readme.com/s/5eaa651315dd18002423bd32

Now if we remove the InputErrorResponse, we'll see that it's resolved:

Screen Shot 2020-04-29 at 10 43 08 PM

Now what if you move the InputErrorResponse schema below NewPreauthorization?

Screen Shot 2020-04-29 at 10 46 16 PM

😬

🚧 Resolution

So the reason this was happening was that since our cleanupSchemaDefaults method for requestBody and component schema objects is recursive, and we were invoking it against the components object, and not individual schemas, it was difficult for us to ascertain when we were first processing a component schema and when we should add a missing type at the top level of a schema if it didn't have one.

For this specific case, what was tripping up our logic was a few things:

  • Both InputErrorResponse and NewPreauthorization schemas didn't have a defined type.
  • Our code would correctly identify InputErrorResponse as an object because we hadn't seen a top-level properties property yet until that point, and when we did we immediately knew that we were processing an object, and could mark it as such.
  • Once we hit the NewPreauthorization schema, and after we had already processed InputErrorResponse, we had the following data in our internal recursive prevProps memory state:
[
  "schemas", 
  "InputErrorResponse", 
  "properties", 
  "code", 
  "message", 
  "NewPreauthorization"
]

Since we retained knowledge of parsing properties, our code misinterpreted this as having already seen this schema before, is it not parsing properties again for it and since it's missing a type to add string so it's still valid JSON Schema.

source

Resolving this involved a couple things:

  • Shift our recursive processing of component schemas to start at the schema itself, not the components level. This way we'll know exactly when we're first seeing a component schema.
  • Update our internal prevProps memory state to clean itself out if we don't have a prevProp. This will resolve the issue of prevProps retaining data from previously processed schemas.

@erunion erunion added the bug Something isn't working label Apr 30, 2020
// If we have a previous prop, but we're parsing the immediate schemas tree in the components object,
// then we're processing a title and description on a component schema.
} else if (prevProp !== false && prevProps.length === 1) {
// If we have a previous prop, but we're parsing the immediate schemas tree (we know this if prevProps

Choose a reason for hiding this comment

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

this stuff is crazy

Copy link
Member Author

@erunion erunion Apr 30, 2020

Choose a reason for hiding this comment

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

All this just to remove this kind of stuff because RJSF renders it as an orphan block of text:

@erunion erunion merged commit 1254603 into master Apr 30, 2020
@erunion erunion deleted the fix/string-type-edgecase branch April 30, 2020 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants