Add remove_client method to base_client.registry.BaseOAuth

[lcdunne]

What kind of change does this PR introduce? (check at least one)

• [ x ] Feature

Feature to support removing a client from the oauth instance (see this issue). This works with the Flask client but I am not familiar with the other frameworks enough to set them up to check. As I mentioned in the issue thread it really looks like those other clients inherit from BaseOAuth but please let me know if I'm overlooking something. I'll try to add what is needed.

---

• [ x ] You consent that the copyright of your pull request source code belongs to Authlib's author.

[lepture]

Hi, thanks. It would be great if you can add a test case for this method.

[lcdunne]

Sure I'll give that a go. Is there a development build that installs all dependencies needed for testing? And any other information on how you are running the tests?

[azmeuk]

The whole testing process is automated with tox. Just run tox, and this will create environments for every python version supported, and a few dependency variants, and this will run pytest on those environments.

[andersnauman]

Can I give a suggestion to change the method name to unregister instead so the naming convention is kept in the same family so to say. Would also suggest the code to be more forgiven for odd behaviors (unlikely with this code but nonetheless a great failsafe).

Code suggestion:

    def unregister(self, name):
        err = None
        if name not in self._registry:
            err = f"Client {name} not found in registry."
        else:
            del self._registry[name]            

        if name not in self._clients:
            err = f"Client {name} not found in clients."
        else:
            del self._clients[name]

        if err:
            raise KeyError(err)

The method create_client first checks if self._clients contains name and if the unregister method do not finish deleting everything, you could end up with a half-deleted state where create_client returns a client that the registry does not have.

[azmeuk]

Can I give a suggestion to change the method name to unregister instead so the naming convention is kept in the same family so to say.

The register naming is not ideal anyways as it could be confused with registration like defined in OAuth 2.0 Dynamic Client Registration Protocol.

In the long run I think keeping remove_client but move register to add_client would be a better idea.

[Jamim]

Hello @lcdunne,

This might be implemented in a bit cleaner and safer way.

Suggested change

	if name not in self._registry:
	raise KeyError(f'Client {name} not found in registry.')
	if name not in self._clients:
	raise KeyError(f'Client {name} not found in clients.')
	del self._registry[name]
	del self._clients[name]
	missing = []
	if not self._registry.pop(name, None):
	missing.append['registry']
	if not self._clients.pop(name, None):
	missing.append['clients']
	if missing:
	raise KeyError(f'Client {name} not found in f{" nor ".join(missing)}.')

Best regards!

[codespearhead]

Also, can you link this PR to that issue ( #583 ), like this?