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

Refactoring auth init and adding tests #133

Merged
merged 10 commits into from
Jul 30, 2024
Merged

Refactoring auth init and adding tests #133

merged 10 commits into from
Jul 30, 2024

Conversation

cbb330
Copy link
Collaborator

@cbb330 cbb330 commented Jul 9, 2024

Summary

problem: 1) auth requirement is not documented in swagger UI 2) auth interceptor init is not tested

(for context, this PR is from a ticket that is meant for me to onboard to the team and learn the code. as part of skilling up I've used refactoring to learn the code better. also, a lot of this code was made in an attempt to add custom headers as types exposed in our client SDK which turned out to be more complexity than it is worth.)

changes:

  • refactored client codegen shell scripts to use a parameterized openapi-codegen cli, unblocking version upgrade (bumped version to 6.6.0
  • logic for converting from clusterproperties yaml -> java objects is pulled out from TablesMvc and into its own util class for testability, refactored for readability.
    • along with move and refactoring, 3 unittests to prevent regression
  • SwaggerConfig.java deleted and merged into TablesMvcConfigurer to both reduce # of places to look for config as well as utilize shared initialization steps. swagger will now init with security documented in UI if security is declared in cluster properties.
  • Authorization is now required in e2e/h2/* tests which enables testing that the springboot app initializes properly with auth interceptor. new unittests for this
  • Automocked MVC will now include Authorization header by default, enabling unittests to use auth required endpoints and without any extra configuration
  • moved AuthorizationPropertiesInitializer to be used in either h2 or mock locations. small unittest for this too
  • AuthorizationPropertiesInitializer now also defines auth interceptor

still TODO:

  • fix naming of getCons (handlerinterceptor class loader)
  • unittest the new HandlerLoader logic
  • add documentation in this PR
  • investigate whether front end docs + back end validation can be consolidated further

Changes

  • Client-facing API Changes
  • Internal API Changes
  • Bug Fixes
  • New Features
  • Performance Improvements
  • Code Style
  • Refactoring
  • Documentation
  • Tests

For all the boxes checked, please include additional details of the changes made in this pull request.

Testing Done

  • Manually Tested on local docker setup. Please include commands ran, and their output.
  • Added new tests for the changes made.
  • Updated existing tests to reflect the changes made.
  • No tests added or updated. Please explain why. If unsure, please feel free to ask for help.
  • Some other form of testing like staging or soak time in production. Please explain.

added several unittests:

  • covering clusterpropertiesutil
  • for proving interceptor initialization is correct and includes headers when needed
  • some small tests for loading clusterproperties from yaml for use in unittests
  • as well as deployed locally to verify that swagger had auth documented on endpoints

Additional Information

  • Breaking Changes
  • Deprecations
  • Large PR broken into smaller PRs, and PR plan linked in the description.

For all the boxes checked, include additional details of the changes made in this pull request.

@cbb330 cbb330 force-pushed the main branch 2 times, most recently from 244c389 to 6a8e224 Compare July 10, 2024 06:09
@cbb330 cbb330 changed the title WIP: Documenting bearer token in API for swagger-ui and code generated clients Refactoring auth init and adding tests Jul 28, 2024
Copy link
Collaborator

@autumnust autumnust left a comment

Choose a reason for hiding this comment

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

minor questions, otherwise LGTM

@cbb330 cbb330 closed this Jul 29, 2024
@cbb330 cbb330 reopened this Jul 29, 2024
@cbb330 cbb330 merged commit fd40280 into linkedin:main Jul 30, 2024
1 of 2 checks passed
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.

2 participants