Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: handle nullable, remove duplication #60

Merged
merged 4 commits into from
Feb 8, 2024
Merged

Conversation

paulsturgess
Copy link
Contributor

@paulsturgess paulsturgess commented Feb 5, 2024

Follow on from #56

There were some places we didn't correctly set nullable for refs (sibling attrs are not allowed for refs).

We can set sibling attrs on refs, if we wrap them in allOf. This PR moves that fix into the central helper that generates most refs and we can enable it by specifying sibling_props: true.

It also fixes an issue where an apia endpoint declares the same error response (error code and http status) more than once. this can happen if say an ArgumentLookup declares an error response and then the endpoint itself does too.

the schema will no longer include endpoints in groups marked with no_schema.
Katapult has some of these for legacy endpoints. And these were also problematic as they used the same names
for a lot of api objects which were causing clashes.

the generator can also now handle when include: true (instead of a string). this is not the correct way to use the option, but Katapult uses it by accident presumably.

partial objects are now only generated when absolutely necessary. previously all child objects of a parent partial object were considered partial and this generated a unique entry in the schema. now we only generate partial child objects when absolutely necessary.

closes: #55

@paulsturgess paulsturgess self-assigned this Feb 5, 2024
field :day_of_week, type: Objects::Day do
field :day_of_week, type: Objects::Day, null: true do
Copy link
Contributor Author

@paulsturgess paulsturgess Feb 5, 2024

Choose a reason for hiding this comment

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

Adding this with no other logic changes made the validation spec fail. So this should ensure we don't accidentally introduce regressions in future.

field :my_partial_polymorph, type: Objects::MonthPolymorph, include: "number", null: true do
description "Partial polymorph"
end
field :my_partial_polymorph, type: Objects::MonthPolymorph, include: "number", null: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the description here to test that we still set nullable correctly even when there is no description for the field.

@paulsturgess paulsturgess requested review from bencromwell and removed request for bencromwell February 6, 2024 16:37
@paulsturgess paulsturgess changed the title fix: ensure nullable is set correctly fix: fix nullable, remove duplication Feb 7, 2024
@paulsturgess paulsturgess changed the title fix: fix nullable, remove duplication fix: handle nullable, remove duplication Feb 7, 2024
Comment on lines -16 to +18
field :time, type: Objects::Time, include: "unix,day_of_week,year[as_string]", null: true do
field :time,
type: Objects::Time,
include: "unix,day_of_week,year[*,-as_integer,-as_array_of_strings,-as_array_of_enums]", null: true do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to make sure we handle negative declarations in the include string

Copy link

@bencromwell bencromwell left a comment

Choose a reason for hiding this comment

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

Whilst I can't attest to the Ruby the end result is where it needs to be! Also appreciate the inclusion of OpenAPI 3.1 labelled comments.

Perhaps in a magical completionist Future where George RR Martin has finished writing his book and the tooling for OpenAPI 3.1 is there, we can use those as markers to update the generation.

@paulsturgess paulsturgess merged commit 1ecfc13 into main Feb 8, 2024
7 checks passed
@paulsturgess paulsturgess deleted the fix-nullable branch February 8, 2024 09:16
@replease replease bot mentioned this pull request Feb 8, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure schema object fields declare if they are nullable
2 participants