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

Support farmOS aggregator #55

Open
paul121 opened this issue Jan 5, 2022 · 4 comments
Open

Support farmOS aggregator #55

paul121 opened this issue Jan 5, 2022 · 4 comments
Labels
enhancement New feature or request

Comments

@paul121
Copy link
Member

paul121 commented Jan 5, 2022

It would be great if this library could also be used to communicate with the aggregator.

I am planning to add an additional "relay" endpoint on the aggregator that can be used to send a request at any given path (not limited to JSONAPI) to an individual farmOS server. In theory, this means farmOS.py just needs to send requests to a different api base path (api/v2/relay instead of api) and be able to include additional query params to identify which farmOS server it intends to communicate with. The aggregator will authenticate these requests accordingly.

farmOS.py will also need to support the api-key authentication that the aggregator uses. The aggregator is also its own OAuth provider (only supporting the password credentials grant), so perhaps farmOS.py could support that (if it doesn't already), but this is less of a priority.

@paul121 paul121 added the enhancement New feature or request label Jan 5, 2022
@paul121
Copy link
Member Author

paul121 commented Jan 5, 2022

Preliminary testing of this is looking quite good! I'm actually able to use farmOS.py to make requests to the aggregator using api-key authentication without any modifications to farmOS.py itself 🎉

This is the script I'm using for testing the new aggregator relay endpoint: https://gist.github.com/paul121/17a3dda773c41074fc71128769cb2193

This works by creating a custom AggregatorSession that subclasses the Requests session (similar to how requests_oauthlib subclasses the requests session). I then implement the methods our client classes use: authorize, http_request and _http_request:

Next, with this custom session created you can manually instantiate any of the client classes (resources, log, asset, etc):

hostname = 'http://localhost'
session = AggregatorSession(hostname)
session.authorize()
resources = ResourceBase(session)

And request logs from the farm_id=1 in the aggregator:

logs = resources.get('log', 'activity', {'farm_id': 1})

@paul121
Copy link
Member Author

paul121 commented Jan 5, 2022

The only thing that does not work with this approach is the resource.iterate() method. This is because it directly makes a request to the links.next URL from the JSONAPI response:

response = self.session._http_request(url=next_url)

This is also using what is supposed to be the "private" _http_request method. Works but just isn't flexible enough for this use case. Instead of directly using this next url we should just use the path and delegate to http_request.

That way subsequent resource.iterate() requests will go through the aggregator, instead of directly to the farmOS server.

** Fixed: f13afa0

@paul121
Copy link
Member Author

paul121 commented Jan 5, 2022

More generally, this gives me some ideas on ways to improve this client library, especially thinking back to #31 and ideas about supporting different session and/or sync/async libraries. The great thing is that, as demonstrated above, this is all largely possible right now by manually instantiating the client resource class with a requests compatible session object!

Thinking forward to a 1.0, 1.1 or maybe 2.0 release (I'm not sure how "breaking" we can be....) I'd like to explore further decoupling of the session and client classes. The client classes could really be tossed out to require uses instantiate separate "resource API" classes themselves - having separate log, asset, etc namespaces is a bit unnecessary anyways, at least until they provide unique additional functionality.

I'm particularly interested in the httpx library which is compatible with the existing requests API but also supports (multiple) async environments

But anyways, as it pertains to this issue and aggregator support, none of this is required now. I'm glad our current abstractions will enable us to explore these ideas further without making changes now.

@paul121
Copy link
Member Author

paul121 commented Jan 5, 2022

I then implement the methods our client classes use: authorize, http_request and _http_request:

Just want to flag this while it is fresh - these http_request and _http_request methods seem redundant. I think it might be an artifact of the 1.x implementation. But one main reason we need this helper method instead of calling session.request or session.get directly is because there is not a way to set a "base url" for the session to always use.

It might be possible to extend the parent session.request method, or perhaps even the prepare_request method. These might be more appropriate (and extendable) than using our own http_request method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

1 participant