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

fix: Minor security schema fix #16

Merged
merged 2 commits into from
Feb 4, 2025
Merged

fix: Minor security schema fix #16

merged 2 commits into from
Feb 4, 2025

Conversation

vblagoje
Copy link
Owner

@vblagoje vblagoje commented Feb 4, 2025

Why:

Enhances the OpenAPI client by ensuring it handles cases where security schemes are not specified, preventing potential issues with missing credentials.

What:

  • Modified the credential handling logic to return a no-op function when security schemes are absent.
  • Added integration test to verify API invocation handling with a real OpenAPI specification.

How can it be used:

The changes allow safer API client initialization in scenarios where security_schemes could be undefined:

if not self.credentials or not security_schemes:
    return lambda security_scheme, request: None  # No-op function

How did you test it:

Implemented an integration test that checks the client's functionality with an actual OpenAPI spec from GitHub's REST API. This includes invoking the API and validating responses.

Notes for the reviewer:

Pay attention to the logic change in get_authenticator to ensure no unintentional behaviors are introduced when security_schemes is not present. The new test depends on the availability of an API key in the environment to run.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 13134513085

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 82.765%

Totals Coverage Status
Change from base Build 13126115354: 0.0%
Covered Lines: 437
Relevant Lines: 528

💛 - Coveralls

@vblagoje vblagoje merged commit 97cc5f7 into main Feb 4, 2025
9 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