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: ADR: Separation of Request and Response schemas #3869

Merged
merged 8 commits into from
Jun 5, 2023

Conversation

nunogois
Copy link
Member

@nunogois nunogois commented May 25, 2023

https://linear.app/unleash/issue/2-943/adr-separate-request-and-response-types-in-apis

During the updating of our OpenAPI documentation, we've seen several times that our schemas are either way too wide for a response or way to strict for a request. This is usually due to us reusing the same schema for both request and response. We should write an ADR where we reason about the usefulness of code duplication and keeping separate response and request schemas.

Based on our needs, this PR adds my suggested ADR.

@vercel
Copy link

vercel bot commented May 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 26, 2023 10:59am
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 26, 2023 10:59am

@nunogois nunogois changed the title docs: add separation of req and res schemas ADR docs: ADR: Separation of Request and Response schemas May 25, 2023

3. **Complexity of migration**: Migrating existing API implementations to adopt the separated schemas might introduce complexities and challenges. Ensuring a smooth transition without disrupting ongoing development or existing integrations requires careful planning and coordination.

4. **Developer adoption and understanding**: Developers who are accustomed to the previous unified schema approach may initially find it challenging to adapt to the new separation of schemas. This could potentially lead to confusion and frustration if the new approach is not clearly communicated and explained.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this document here for communication purposes? Seems odd to say this may be a problem in the very document that's intended to address the problem

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with Simon here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, makes sense. Addressed (yeeted) in 0490483


2. **Data duplication and redundancy**: With the separation of schemas, there might be instances where certain data fields or structures are duplicated between the request and response schemas. This redundancy could lead to code duplication and increase the risk of inconsistencies if changes are not carefully synchronized between the two schemas.

3. **Complexity of migration**: Migrating existing API implementations to adopt the separated schemas might introduce complexities and challenges. Ensuring a smooth transition without disrupting ongoing development or existing integrations requires careful planning and coordination.
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, the plan is here is do liberally apply the boyscout rule here. If that's the case then I think it's unlikely we'll do a complex migration. Only that new changes are required to incorporate this decision

Copy link
Contributor

Choose a reason for hiding this comment

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

True, I do not believe a big bang migration replacing all schemas at once is feasible. But applying the boyscout rule (leave things better than you found it) would mean eventually all code that is in use will be updated to separate the schemas.

We should still aim to complete the migration by the release of 6.0

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I yeeted this concern and added some details about our migration strategy on the conclusion:


The separation of request and response schemas will provide the following benefits:

1. **Enhanced clarity and correctness**: With dedicated schemas for requests and responses, we can define the precise structure and constraints for each interaction. This will help prevent situations where the schemas are overly permissive or restrictive, improving the accuracy and clarity of the API documentation.
Copy link
Member

Choose a reason for hiding this comment

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

For me, the correctness and maintainability is the heart of the value here. And I think this spans more than just the documentation, the unified schemas make writing code handling code much tougher and mean we're passing a lot of undefineds around where they aren't necessary. I'd like to see this fleshed out a little here. Personal feeling perhaps

Copy link
Contributor

Choose a reason for hiding this comment

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

I concur, the correctness is my main motivation for driving this. It's been annoying to me that we can't claim that we know the shape of a response object because we risk failing to handle a request object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, tried expanding a bit on it here: 7bbacb8


## Conclusion

By implementing the separation of request and response schemas, we aim to improve the robustness and maintainability of our API documentation. This decision will empower developers to build more reliable integrations by providing clearer guidelines for both request and response data structures.
Copy link
Member

@sighphyre sighphyre May 26, 2023

Choose a reason for hiding this comment

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

I think it's worth mentioning that this also has value to us, internally. This should allow us to write more robust code and do less massaging of incoming requests into the correct shapes. That in turn reduces the likelihood of bugs and makes the code significantly easier to reason about and work with

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I think we're conflating terms here. Incoming responses doesn't make sense to me. We are talking incoming requests and outgoing responses no?

The point still stands that preparing responses will be easier, and requests will accept anything, but have a predefined set of fields we'll actually need to look at, any superfluous fields can just be dropped.

Copy link
Member

Choose a reason for hiding this comment

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

Yuh, thanks Chris! I meant to say incoming requests, edited my original comment to reflect that, leaving this here so that your comment makes sense to people who come after

Copy link
Member Author

@nunogois nunogois May 26, 2023

Choose a reason for hiding this comment

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

Thanks, tried addressing this here: 93f062c

…response-schemas.md

Co-authored-by: Simon Hornby <liquidwicked64@gmail.com>
@thomasheartman
Copy link
Contributor

Just wanted to chime in and say that I think this looks great! Super job, @nunogois 🙌🏼 I agree with all of Simon's points, but otherwise have no further input. Just happy to see this coming together! 😄

@nunogois nunogois requested a review from sighphyre May 26, 2023 10:57
Copy link
Contributor

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank you Nuno for doing the work.

@nunogois nunogois merged commit 6b4efb9 into main Jun 5, 2023
16 checks passed
@nunogois nunogois deleted the docs-adr-req-res-schemas branch June 5, 2023 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants