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

Does this work in current state? #6

Closed
saward opened this issue Nov 1, 2022 · 14 comments · Fixed by #7
Closed

Does this work in current state? #6

saward opened this issue Nov 1, 2022 · 14 comments · Fixed by #7

Comments

@saward
Copy link

saward commented Nov 1, 2022

I had to make a change in my fork to be able to connect, otherwise I was getting 404 errors (trying the assets endpoint). The change I made was in client.rs to use the BASE_URL for all the API endpoints, rather than using the OAUTH_URL when authenticated.

Just wondering if something's changed, or if I'm misusing it?

@Celeo
Copy link
Owner

Celeo commented Nov 1, 2022

Heya, thanks for the interest. I'll look into it!

@Celeo
Copy link
Owner

Celeo commented Nov 1, 2022

I'm guessing it's this.

@Celeo
Copy link
Owner

Celeo commented Nov 1, 2022

I'll update the library to work with the new flow.

@saward
Copy link
Author

saward commented Nov 1, 2022

Thanks, sounds good. I may have some contributions to push back to this project too. If this gets fixed, then it's a bit easier so that I haven't diverged too far.

Also, is there any built in way to use the refresh tokens after the access token expires, or is that not built yet? I couldn't see any code that handles refreshing.

@Celeo
Copy link
Owner

Celeo commented Nov 3, 2022

Give 0.12.0 a try. Thanks for reporting!

@saward
Copy link
Author

saward commented Nov 3, 2022

Thanks, will probably give it a whirl this weekend :) I also had a question about pagination of results. I was looking at that and trying to work out the best way to support it. Is there a good way to chat with you about designs for this sort of thing?

I notice with markets there's a way to add page to the query, but I'm thinking about a way to read the header for how many pages there are, and to return a vec with all the results. Not sure of the best way to fit it in to the current design if I were to make that change and submit a PR.

@Celeo Celeo reopened this Nov 3, 2022
@Celeo
Copy link
Owner

Celeo commented Nov 4, 2022

Like here? Looking at the ESI endpoint for "get_markets_region_id_orders" (/v1/markets/{region_id}/orders/), it takes a "page" parameter in the URL like the other parameters. What isn't currently supported is getting back that "x-pages" header to denote how many pages are available.

@saward
Copy link
Author

saward commented Nov 4, 2022

Yes, that's it. I was thinking of submitting a PR with a way to automatically handle x-pages, but I haven't thought of a good way to do it yet that won't be a breaking change. I think the request would need to either (a) have a variant of the macro that iterates over the results and returns a vec, or (b) return information about total pages so that the caller has the info they need to call pages without over-requesting.

I can do things by hand for each endpoint, by incrementing pages until I get a 404 error back (iirc), but that's less neat. It's not too bad though.

@saward
Copy link
Author

saward commented Nov 4, 2022

I saw you added refreshing the token, which is awesome. Some suggestions (and maybe I'll submit PR's for these myself if I keep going on with this):

  • It looks like the refresh request can return a new refresh token, which from reading this new change doesn't look like it's being read (RefreshTokenAuthenticateResponse): https://docs.esi.evetech.net/docs/sso/refreshing_access_tokens.html#sso-response
  • Would it be possible to have the library automatically use the refresh token if the access token is expired, rather than needing to explicitly refresh?
  • Maybe allowing the EsiBuilder to be serialised/deserialised so that it can be stored in a database easily, and pulled out again (not sure if I'd use this, but might be as easy as adding a serde derive for serialize/deserialize)

@Celeo
Copy link
Owner

Celeo commented Nov 4, 2022

refresh request can return a new refresh token

Oh, great catch! Despite that text being bold, I totally missed it. Easy change.

automatically use the refresh token

Sure could, great idea.

serialised/deserialised ... easy as adding a serde derive for serialize/deserialize

Yeah I think so too. Definitely easier to do the builder than the final struct!

@Celeo
Copy link
Owner

Celeo commented Nov 4, 2022

Refresh token fix: 2caa909

Builder (de)serialization: 555cf69

@Celeo
Copy link
Owner

Celeo commented Nov 5, 2022

Some better access token expiration handling is in 41ec152, but to auto-refresh on expiration, that function will have to take &mut self, so macros and such need to be updated, which I can look into soon.

@saward
Copy link
Author

saward commented Nov 5, 2022

Thanks, you're very quick to respond!

@Celeo
Copy link
Owner

Celeo commented Nov 7, 2022

Continued on in #8

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 a pull request may close this issue.

2 participants