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

9540: add committee-rails gem to gemfile #18735

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kpethtel
Copy link
Contributor

@kpethtel kpethtel commented Oct 3, 2024

Summary

Adds a the committee-rails gem to the vets-api. The gem will be used to validate request specs againt openapi documentation. Subsequent PRs will add use of the gem.

Related issue(s)

department-of-veterans-affairs/va-mobile-app#9540

Testing done

Screenshots

What areas of the site does it impact?

None

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.
  • Events are being sent to the appropriate logging solution
  • Documentation has been updated (link to documentation)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • Feature/bug has a monitor built into Datadog (if applicable)
  • If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
  • I added a screenshot of the developed feature

Requested Feedback

Copy link
Contributor

@LindseySaari LindseySaari left a comment

Choose a reason for hiding this comment

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

Hey there! You will need to follow the guidance here around adding a new third party package.

@stevenjcumming
Copy link
Contributor

stevenjcumming commented Oct 3, 2024

@kpethtel
Copy link
Contributor Author

kpethtel commented Oct 4, 2024

After talking to @stevenjcumming this morning, I believe we can rule out the json-schema gem because it doesn't understand openapi formatting. I'm still open to rswag if anyone knows how to use it to validate against yaml/json/html.

Here are my responses to the checklist (cc: @LindseySaari):

How recently was the package updated? (should be recent)
The committee-rails gem was updated 8 months ago, but it's primarily an adapter for the committee gem so it probably doesn't require frequent changes. The committee gem was updated 3 weeks ago.

Is the package still actively maintained? (should be)
it appears to be

Does the package have a large number of downloads? (Low download numbers can be indicative of a poor package).
Committee-rails: 4,114,755
Committee: 7,509,353

Does the package currently have any, or has had a history of security vulnerabilities? (Security is paramount)
Looking through the committee-rails and committee gems' issues, I don't see any security issues.

Is there another library on VA.gov that already does this function? (duplicating code becomes a drag on performance)
Not that I'm aware of. It's possible rswag can accomplish the same task, but it's not clear to me from reading the docs how one would do it. Also, this doesn't appear to be rswag's primary function. Rswag is primarily for generating docs, not for validating request specs against yaml/json/html.

Has another team possibly written custom functionality to solve for the absence of this library, that can be shared instead of installed separately?
I don't think so.

Is there another library that might be better suited for this purpose? One that’s more secure or better supported? One that’s more flexible?
Not that I can find, but I'm open to suggestion.

Background on the library.
This library is intended for validating request specs against openapi documentation. My team (mobile) wants to do this because:

  • we frequently find errors in our documentation
  • developers are human and often forget to update documentation when the underlying code changes
  • other teams have the ability to make changes that impact our endpoints and this will reduce the chances that they're breaking our schema. developers from other teams will also be adding features to our code and this will provide better guardrails.

What functionality it’s going to be assisting in performing.
schema validation and preventing schema drift

Why it’s necessary as opposed to a lighter solution.
I have explored the possibility of creating a custom solution by writing functions that stitch together the schema by recursively replacing all $refs with the actual referenced schema then using JSON::Schema to handle the actual validation. I do believe this is viable, but it is also more fragile and I think it may run into issues regarding openapi formatting. For example, openapi uses nullable: true whereas the JSON::Schema expects "type": "string, null". There may be other differences that will cause more substantial issues.

Documentation of research performed including the docs for the library, packages it was compared to, and why this solution was ultimately chosen.
https://github.com/willnet/committee-rails
https://github.com/interagent/committee
https://github.com/rswag/rswag
https://github.com/ota42y/openapi_parser
https://github.com/westfieldlabs/apivore

I looked at several other gems including JSON::Schema and Rswag but didn't find any others that fit this exact business case. I also attempted to use the openapi parser gem to parse our yaml files in order to use the JSON::Schema gem to validate them, but I ran into issues getting it working with yaml and that gem is used by committee anyhow. I also looked at apivore, which is used in the vets-api, but as far as I can tell the gem hasn't been updated in 8 years and does not support openapi3.

Copy link

github-actions bot commented Oct 7, 2024

Backend-review-group approval confirmed.

Copy link
Contributor

@rmtolmach rmtolmach left a comment

Choose a reason for hiding this comment

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

Waiting for PO/security approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants