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

docs: Fallback to reference description for responses #826

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

Nelspike
Copy link
Contributor

@Nelspike Nelspike commented Oct 4, 2024

Checklist

This PR attempts to resolve the issue on #795 by resolving the $ref given in the schema and fetching that description in case there's none. PTAL! 🚀

@Nelspike Nelspike mentioned this pull request Oct 4, 2024
2 tasks
Copy link
Contributor

@jean-michelet jean-michelet left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I don't know the code base so I can't approve myself.

lib/spec/openapi/utils.js Outdated Show resolved Hide resolved
lib/spec/openapi/utils.js Outdated Show resolved Hide resolved
lib/spec/openapi/utils.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jean-michelet
Copy link
Contributor

Does it solve your bug @melroy89?

@melroy89

This comment was marked as outdated.

@Nelspike
Copy link
Contributor Author

Nelspike commented Oct 7, 2024

First of all, @jean-michelet, thank you for the review! 🚀

Secondly and equally thankful to @melroy89 for the detailed reproduction description 💪🏼 From what I can see from your code, the only reason why this wouldn't work is due to the trailing # on your $ref, which is notFoundError#. That $ref is absolutely correct, so we need to find a way to resolve these properly in this PR. If you want to give it a try without the trailing #, I think that would do the trick, at least for this example!

@melroy89

This comment was marked as outdated.

@Nelspike
Copy link
Contributor Author

Nelspike commented Oct 7, 2024

@melroy89 - That's exactly what I meant in my reply, we need to consider those use cases because your reference is 100% correct 😅 The code I pushed in the PR didn't take that into account yet, but just pushed a change that does! I never meant that you should remove #, but that the code I had previously worked without it.

@Nelspike
Copy link
Contributor Author

Nelspike commented Oct 7, 2024

Seems like the second use case you mentioned on your EDIT is blocked by this, @melroy89: #676

At the moment, seems like definitions is not supported within OpenAPI definitions on fastify-swagger, which difficults using refs like commonResponse#/definitions/notFound. However, your first use case with # at the end should be covered by this PR now 👍🏼 Once the other one's merged, it should be supported out of the box!

cc @asc11cat @Eomm @ivan-tymoshenko, tagging you folks for visibility since you were discussing on the other PR 😄

@melroy89
Copy link
Contributor

melroy89 commented Oct 7, 2024

However, your first use case with # at the end should be covered by this PR now 👍🏼 Once the other one's merged, it should be supported out of the box!

Let me test it again..

@melroy89
Copy link
Contributor

melroy89 commented Oct 7, 2024

Yup works now! Thank you so much! I was looking for this for years.

@Nelspike I would like thank you by providing a donation! Any payment method will do.

@Nelspike
Copy link
Contributor Author

Nelspike commented Oct 8, 2024

@melroy89 - Thank you for your generosity and recognition! I'm totally okay doing this without a donation whatsoever, happy to help wherever I can 🎉

@melroy89
Copy link
Contributor

melroy89 commented Oct 8, 2024

@melroy89 - Thank you for your generosity and recognition! I'm totally okay doing this without a donation whatsoever, happy to help wherever I can 🎉

OK, if you change your mind. Let me know.

@melroy89
Copy link
Contributor

melroy89 commented Oct 9, 2024

Now hopefully this PR gets merged somewhere before 2030 :)

@jean-michelet
Copy link
Contributor

Now hopefully this PR gets merged somewhere before 2030 :)

I don't think this type of comment is going to help us move any faster. If @climba03003 is ok with the code, I will merge it.

Signed-off-by: Gürgün Dayıoğlu <hey@gurgun.day>
@melroy89
Copy link
Contributor

melroy89 commented Oct 9, 2024

Now hopefully this PR gets merged somewhere before 2030 :)

I don't think this type of comment is going to help us move any faster. If @climba03003 is ok with the code, I will merge it.

No one can take a joke anymore.

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

Haha don't worry @melroy89, I assure you we'll get this done this year :)

I just need another member (@climba03003 would be golden) to review

README.md Outdated
@@ -468,6 +468,9 @@ fastify.get('/responseDescription', {
}
}, () => {})
```

Additionally, if you provide a `$ref` in your response schema but no description, the reference's description will be used as a fallback.
Copy link
Member

@climba03003 climba03003 Oct 10, 2024

Choose a reason for hiding this comment

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

Please state the $ref currently only support the format of matching $id.
It cannot lookup the fragment part. For example, schemaId#/definitions/subSchema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good one, would pointing to the PR that's currently in the works (#676) suffice? 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in a82f2a7, @climba03003 - Feel free to take another look!

README.md Outdated Show resolved Hide resolved
Co-authored-by: KaKa <23028015+climba03003@users.noreply.github.com>
Signed-off-by: Gürgün Dayıoğlu <hey@gurgun.day>
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

I can release tonight, if anyone's free, feel free to merge and release

@jean-michelet jean-michelet merged commit ac99039 into fastify:master Oct 11, 2024
11 checks passed
@jean-michelet
Copy link
Contributor

Release tonight plz @gurgunday!

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.

5 participants