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

Update the API class to properly put the access_token into the header. #535

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

anton-molyboha
Copy link
Contributor

This pull request:

  • Creates an Api_new class that mirrors the old Api class, but
    1. All api functions return a namedtuple HttpRequest, that includes headers information.
    2. All authorization is done via "Authorization" header rather than the query parameter.
  • Migrate AsyncClient and HttpClient to use the API_new class.

This should be a non-breaking change at this point. I have run the existing tests, and they pass, but I did not try to improve test coverage; bugs could slip through the cracks.

The next steps would be to:

  1. Rename Api_new -> Api, while having the old Api class available as Api_old.
  2. Delete Api_old.
    Those will be braking changes for those, who depend directly on the Api class, but users will have a path to upgrade.

@anton-molyboha
Copy link
Contributor Author

Forgot to mention the related discussion: #511

@anton-molyboha anton-molyboha force-pushed the main branch 2 times, most recently from 948d856 to a831c6e Compare February 9, 2025 12:06
Api_new differs from Api_old in that:
 1. All api functions return the same type, HttpRequest
 2. All authorization is done via "Authorization" header rather than the
    query parameter.
Copy link
Collaborator

@PaarthShah PaarthShah left a comment

Choose a reason for hiding this comment

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

At a high level, I think we should avoid all _new naming conventions, both for classes and functions. The _old pattern is fine, but I'd make a point of using the warning stdlib to introduce a deprecation warning.

There are a few reasons for this:

  1. It's ugly and bloats the number of changes. I can't review this properly in this state, but I'll be willing if it goes down to half the size or less.
  2. Anyone downstream that may have overridden these classes/names is in for a bad time. Leaving the names the same means there's a good chance that everything will continue to work as long as they invoke super(). If they do depend on existing behavior, then let them pin an older version, or update their code.
  3. Considering how long the matrix spec has supported the use of these headers, I don't have any serious concerns that we need to worry about backwards compatibility with non-compliant servers
  4. By not being at its 1.0 release, matrix-nio itself (by way of semver) isn't making any promises of being perfectly backwards compatible, and for something this low level, I presume nobody would actually notice this difference in behavior (this relates to both 3 and 4).

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