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

Fix lost skip configurations in allof blocks and readOnly and writeOnly in examples #141

Closed
wants to merge 5 commits into from
Closed

Fix lost skip configurations in allof blocks and readOnly and writeOnly in examples #141

wants to merge 5 commits into from

Conversation

NickUfer
Copy link

@NickUfer NickUfer commented Jun 12, 2022 •
edited

What/Why/How?

The sampler ignores readOnly and writeOnly on properties in allOf's and in examples.

The problem with allOf's is that the readOnly and writeOnly information of properties is lost in them. This PR preserves the information by marking the properties with a special object MARKED_FOR_REMOVAL which were supposed to be removed by skipNonRequired=true, skipReadOnly=true or skipWriteOnly=true. Before the result is returned all values which are supposed to be removed are removed.

The problem with having example values in the sample even if they are readOnly or writeOnly is fixed by traversing the schema again for which the example is intended and removing all values which are invalid from the example.

Reference

Fixes #140
Fixes Redocly/redoc#1238

Testing

Tests for the changes were added.

Screenshots (optional)

Check yourself

  • Code is linted
  • Tested
  • All new/updated code is covered with tests

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

@NickUfer NickUfer changed the title Fix lost skip configurations in allof blocks Fix lost skip configurations in allof blocks and readOnly and writeOnly in examples Jun 12, 2022
AlexVarchuk
AlexVarchuk requested changes Jun 20, 2022
View reviewed changes
Copy link
Contributor

@AlexVarchuk AlexVarchuk left a comment

Choose a reason for hiding this comment

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

Hi @NickUfer . Thank you for your contribution. We appreciate it. 🙌

I think, it is too many changes and too complicated to fix this problem. It looks like a workaround fix. With additional steps to filter all keys 🙈 .

The bug is in the allOf.js file.
We'll get the example value of each item of allOf and merge the result value after it.
Seems this behavior is not correct.
We should resolve allOf to a flat schema and after that generate an example or think about a better solution.

Can you simplify it instead of marking keys for removal?

Also, we should not ignore some example properties from the schema.
It is related to additionalProperties keyword in the schema. They are allowed by default.

@NickUfer
Copy link
Author

Simplification is IMO not possible because the architecture of the sampler project is flawed. Crucial information about properties is lost in the process and IMO a rewrite is necessary to fix this, which includes keeping as much information as possible until the final merging of a single object. That's why I decided to write this fix, even if it is not exactly the perfect solution.

About the additionalProperties, yes that is a problem. Maybe the example filtration should be disabled by default and only enabled when the schema includes additionalProperties: false

@NickUfer NickUfer closed this Sep 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexVarchuk AlexVarchuk AlexVarchuk requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@NickUfer @AlexVarchuk