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

Question regarding farmOS Client #1

Closed
paul121 opened this issue Jan 9, 2020 · 8 comments
Closed

Question regarding farmOS Client #1

paul121 opened this issue Jan 9, 2020 · 8 comments

Comments

@paul121
Copy link

paul121 commented Jan 9, 2020

Wow @symbioquine you've really done a lot of work since I last looked at this repo!

Just a couple questions out of curiosity regarding how you're handling requests to the farmOS server. Last I saw you were using the farmOS.py library and I was curious how that was working... but now I see the custom(?) Drupal RestWS Client! Neat!

Ironically, I just today learned about the geojson api at /farm/areas/geojson. I see you had a work around at one point using this:
areas = farm.session.http_request("farm/areas/geojson").json()
But are you still using this API? It seems like it is "all or nothing" regarding the data it returned.

It sounds like motivation was also because farmOS.py (requests) is blocking. Did that create some headaches when working with the WFS side of things? Once we got in deeper with the aggregator I saw how great it would be if farmOS.py was non blocking... 😄

  • I had been looking at requests-async but am just now seeing that is is archived and recommends httpx.
  • But twisted looks more than capable as well!! haha
  • I've been working with fastAPI quite a bit for the Aggregator which is built with the asyncio starlette toolkit. This has the same maintainers as httpx so I'm a little inclined to use that in the future, although it has a bit more limited support for async right now

Would love to hear your thoughts!

@symbioquine
Copy link
Owner

Yeah @paul121, I ended up rewriting the FarmOS client for my purposes. I wanted;

  • Fully non-blocking
  • Better handling of credential refresh by making the new client aware of the cookie expiry
  • Better separation of concerns between the "Drupal layer" and the "FarmOS layer"
  • Cleaner pagination semantics

It would be great to collaborate and see whether we could offer these things in the official FarmOS library in both blocking and non-blocking flavors. (Though I'm not sure it would be easy to get from where the library is now to that state in a fully backwards-compatible way.)

I'll have to check out those other libraries (requests-async, fastAPI, httpx, starlette). It looks like Pythons async IO options have come a long way since I learned Twisted.

@paul121
Copy link
Author

paul121 commented Jan 9, 2020

Better handling of credential refresh by making the new client aware of the cookie expiry

Good point. I've mostly been thinking about expiration for OAuth and didn't consider the session cookie expiration. With OAuth I did have to implement a method of retrieving a csrf_token but I'm not aware of an expiration with that? (Also curious how this is done in farmOS.js @jgaehring)

Better separation of concerns between the "Drupal layer" and the "FarmOS layer"

The separation between the Drupal and farmOS layer makes a great bit of sense. @jgaehring and @mstenta might be interested in seeing that - https://github.com/symbioquine/farm-os-area-feature-proxy/blob/master/src/tx_drupal_rest_ws_client.py. Although it isn't necessary now, it might be worth considering in the future to support more "custom" modules that are more tightly integrated with Drupal than farmOS.

Cleaner pagination semantics

Interesting. When we started the client libraries I was mostly following what we had in place with farmOS.js, which came from an early draft PHP library. But I remember we chose to pass on the server response which includes the pages in an object similar to {page: 1, first: 1, last: 4, list: []}. This allows the user control to evaluate the response and make another request with farm.log.get(page=2) (or use the default return all pages)

Anyways, I've been curious if having a page.next() would make farmOS.py a bit more "pythonic". I thought returning an iterator over the list object might be handy, too. I think that could facilitate automatic paging of results, if the call for the next element requires another request from the server. It seems like this is where the API for the Python and JS libraries could differ some, since it's just a matter of how to best use them in their respective languages

It would be great to collaborate and see whether we could offer these things in the official FarmOS library in both blocking and non-blocking flavors. (Though I'm not sure it would be easy to get from where the library is now to that state in a fully backwards-compatible way.)

Yeah, I'm definitely interested in what we can do here. If we can avoid creating farmOS-async.py that would be great 😄

@mstenta
Copy link

mstenta commented Jan 9, 2020

Better separation of concerns between the "Drupal layer" and the "FarmOS layer"

Curious what you mean by this.

(Though I'm not sure it would be easy to get from where the library is now to that state in a fully backwards-compatible way.)

Worth noting that the farmOS 2.x upgrade will be a BC-breaking change to the API, so that's an opportunity to make changes like this.

@jgaehring
Copy link

The separation between the Drupal and farmOS layer makes a great bit of sense. @jgaehring and @mstenta might be interested in seeing that

Hmm, what are we looking at here specifically, not sure I fully understand.

@mstenta
Copy link

mstenta commented Jan 10, 2020

Better separation of concerns between the "Drupal layer" and the "FarmOS layer"

Chatted with @paul121 and I think I understand what this was referring to now. The python script @symbioquine created (https://github.com/symbioquine/farm-os-area-feature-proxy/blob/master/src/tx_drupal_rest_ws_client.py) has more generalized functions for working with Drupal entities, rather than the higher level functions for dealing with assets, logs, etc.

I could see that being something that would be useful to other Drupal projects/users more generally. And if a general Drupal python API library existed, perhaps farmOS.py could extend from it. Doesn't look like something like that exists currently. However, there are a lot of ways to set up APIs in Drupal 8, so it might not be worth the effort to maintain a general-purpose library. Still, it might be useful to adopt some general methods in farmOS.py which the higher-level methods can call.

@symbioquine
Copy link
Owner

Cleaner pagination semantics

Interesting. When we started the client libraries I was mostly following what we had in place with farmOS.js, which came from an early draft PHP library. But I remember we chose to pass on the server response which includes the pages in an object similar to {page: 1, first: 1, last: 4, list: []}. This allows the user control to evaluate the response and make another request with farm.log.get(page=2) (or use the default return all pages)

@paul121 It feels messy for the client of the library to need to know how the pages are indexed or keep track of how many pages there are.

In other words I prefer;

page = yield drupal_client.get_entities(entity_type, filters)
while page:
    # Do something with page
    page = yield page.next_page()

over;

logs = farm.log.get()
# Do something with page
while logs.get('self') < logs.get('last'):
    logs = farm.log.get(page=logs.get('self') + 1)
    # Do something with page

Anyways, I've been curious if having a page.next() would make farmOS.py a bit more "pythonic". I thought returning an iterator over the list object might be handy, too. I think that could facilitate automatic paging of results, if the call for the next element requires another request from the server.

It should somehow be explicit. Something like this would be nice (in a synchronous API anyway);

for entity in drupal_client.iterate_entities(entity_type, filters):
    # Do something with entity

It seems like this is where the API for the Python and JS libraries could differ some, since it's just a matter of how to best use them in their respective languages.

Totally agree. API semantics tend to differ pretty widely between languages.

Worth noting that the farmOS 2.x upgrade will be a BC-breaking change to the API, so that's an opportunity to make changes like this.

Cool!

However, there are a lot of ways to set up APIs in Drupal 8, so it might not be worth the effort to maintain a general-purpose library. Still, it might be useful to adopt some general methods in farmOS.py which the higher-level methods can call.

@mstenta Yeah I ran pretty sharply up against that when I went to pull out the Drupal part into its own repository. The immediate next question is, "how will this work long term?" From what I've seen so far, I suspect D8 provides too much flexibility for how its APIs can be configured to the point where it is pretty hard to have sane default behavior for a client library - at least without some serious "meta" APIs which provide data about how it is configured.

@mstenta
Copy link

mstenta commented Jan 10, 2020

@symbioquine Agreed. And in farmOS's case, we are making a lot of those design decisions on top of Drupal's flexible base, so we can provide a more "defined" client library in that regard.

@symbioquine
Copy link
Owner

@paul121 @mstenta

It looks like Python support for async has come a long way since the style I was familiar with from Twisted.

I will put more thoughts in farmOS/farmOS.py#31.

Closing this issue since I suspect further development of a farmOS client as an implementation detail of farm-os-area-feature-proxy would be silly.

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

No branches or pull requests

4 participants