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

make auth_header optional to enable using _request_session.headers #632

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

Conversation

zymergy
Copy link

@zymergy zymergy commented Oct 12, 2022

Before this PR

requests provides a convenient Session object that allows headers to be shared across all requests made with a given session. conjure-python initializes all services with a requests.Session() object, but also created a non-optional auth-header field for all generated methods that require header authentication. Even if the auth header is specified in the Session object, it will not be respected and must be provided for every method call. This PR modifies the generation code to ensure that headers are optional, and will override the Service._request_session.headers object only if specified

After this PR

==COMMIT_MSG==
Required headers are now optional when calling methods and can instead be provided in the requests.Session.headers dict. If headers are provided in the method, they will override the session headers.
==COMMIT_MSG==

Possible downsides?

The exception raised for missing required headers will change from a TypeError to the exception raised by requests

@palantirtech
Copy link
Member

Thanks for your interest in palantir/conjure-python, @zymergy! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@changelog-app
Copy link

changelog-app bot commented Oct 12, 2022

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Required headers are now optional when calling methods and can instead be provided in the requests.Session dict. If headers are provided in the method, they will override the session headers.

Check the box to generate changelog(s)

  • Generate changelog entry

@zymergy
Copy link
Author

zymergy commented Oct 12, 2022

I suspect it would be helpful to add documentation pointing out that any headers in the method parameters are REQUIRED headers, they just don't have to be specified in the method call itself

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