Shared http client

[zhemaituk]

This PR includes couple of improvements:

• ability to pass a http client to use. Useful for the cases when using library to work with multiple accounts (different refresh tokens), and benefit from common http collection pool, keep alive, etc. For this reason oauth handling moved to HttpTdaClient so the http client is not bound to specific refresh token.
• ability to pass access token if known (optional)
• placeOrder returns id of the created order
• RuntimeException replaced by a more specific one
• Minor dependencies upgrade (addresses https://nvd.nist.gov/vuln/detail/CVE-2021-28170 and alike)

[zhemaituk]

@studerw can you take a look by chance please?

[studerw]

Hi @zhemaituk i will have time to look at this later this week. Thanks for the PR!

[zhemaituk]

Any chance for review @studerw please?

[zhemaituk]

@studerw any chance for merge or feedback? If this project is abandoned let me know also, I'll stop trying and will keep the changes just in my fork.

[oldcatpiss]

Does this PR resolve this issue?

When passing client - 1st request fails, but 2nd works? Seems like it does

[main] INFO com.studerw.tda.client.HttpTdaClient - Fetching Fundamental Instrument data with id: ACABU [main] INFO TDA_HTTP - REQUEST GET: https://api.tdameritrade.com/v1/instruments?symbol=ACABU&projection=fundamental [main] INFO TDA_HTTP - RESPONSE [401 - Unauthorized]: https://api.tdameritrade.com/v1/instruments?symbol=ACABU&projection=fundamental in 422.6 ms [main] INFO TDA_HTTP - REQUEST POST: https://api.tdameritrade.com/v1/oauth2/token [main] INFO TDA_HTTP - RESPONSE [200 - OK]: https://api.tdameritrade.com/v1/oauth2/token in 140.5 ms

[zhemaituk]

@oldcatpiss it was addressing it just partially (only if you supply access key in input properties). But updated the PR to address that as well fully, if no access key is known it will try to retrieve one, instead of failing first.

Btw, for the time being I am releasing my changes to maven central under this group id:

<dependency>
    <!-- TODO: Switch to original groupId after https://github.com/studerw/td-ameritrade-client/pull/54 merged. -->
    <!-- groupId>com.studerw.tda</groupId -->
    <groupId>com.breakingequity</groupId>
    <artifactId>td-ameritrade-client</artifactId>
    <version>2.4.2.3</version>
</dependency>

[studerw]

@zhemaituk @oldcatpiss

The way the original design works was that it will ALWAYS fail on the first call because, by design, the interceptor catches authentication issues and tries to fix it - it offloads that responsibility from the main client in order to massively simplify its design.

Yes it is known that this will ALWAYS waste the first call which will fail fast, but it makes things a lot more simple in the many corner cases where, at any time - you have no idea because it's determined by the server - your auth session may expire or time out or be reset. Again, if you handle all that initial authentication logic within the client itself to avoid that first failure call, then you still need all that extra logic to handle random session expirations.

Without having to dig through your code, what was your solution @zhemaituk?

Theoretically, you could keep some state variable that knows if we've yet to create an initial session and pre-fetch it to avoid that first failure call, but then you have to deal with concurrency issues (e.g. multiple threads using the client initially at once, and then overriding each other's sessions) which creates more problems than it is worth, IMO. The way my auth interceptor works it shouldn't have that problem because that session info is handled by the OKCupid client, which is thread safe by design.

[oldcatpiss]

Yeah, no biggie - I just noticed the pr and gave it a look. Thanks for the response! Great client and Thanks for sharing! @studerw

[zhemaituk]

@studerw conceptually similar to what you had, with difference that we generate token if we know it's not present: 1) Do not do tdaProps.setProperty(ACCESS_TOKEN, "UNSET"); in constructor. If user supplied own access token will try the supplied token, otherwise keep as null. 2) When making http request: tdaProps.computeIfAbsent(ACCESS_TOKEN, key -> fetchNewAuthToken());. Concurrency is addressed by ConcurrentHashMap, it will compute new value if key is absent. 3) If when making http request we get 401: reset the token via tdaProps.remove(ACCESS_TOKEN, token); (Concurrency is addressed by ConcurrentHashMap here as well, it does same validations as you had in your code, validating that the token has not been modified by a different thread in the meantime) and retry the request again as in step 2).

[studerw]

@zhemaituk I did finally get around to updating some of the dependencies that were failing security scans and I also cherry picked your code to return the order ID using the location header. Thanks for that code!

As for some of the other more complex stuff to do with authentication, I'm not even sure it is worth the effort. FYI, TDA was bought out by Schwab, and supposedly they're migrating users to a different API they may or may not be compatible with this version, assuming it will actually exist some day.

It was supposed to happen this year, but their docs and notices are very sparse.

Regardless, this is the 2nd time in 5 years they've completely screwed over users and devs who counted on their "personal" API access, and I'm not real interested in wasting any more time & effort with TDA (or whichever megacorp currently owns them). 🤷

[zhemaituk]

Makes sense @studerw. Once Schwab API is available we can implement something good for that one. Also @alexgolec from https://github.com/alexgolec/tda-api/ mentioned in his discord an intent to implement Java client along with Python client once Schwab API is available, maybe we can team up.

On Schwab API timelines, latest from their support:

We expect registration for legacy applications to be available within the next month (July 2023). The first set of endpoints will be live later on this summer and full launch of the new api is expected in the Winter of 2023.

On location header with order ID: I found it to be helpful, but not that reliable for a few reasons:

• It might return wrong order ID. For example while submitting new order to the API, another order executed and triggered a stop loss leg. The API might return order ID of that unrelated stop loss leg, instead of ID of the submitted order.
• In case of network interruption, there is no reliable way to validate if the order was or was not placed. For these reasons in client code (not in td-ameritrade-client lib itself, but where the lib is used) I had to add logic to retrieve just created order by id and compare if it looks the same as the order submitted, and if not try to do recovery (list recent orders and look among them for a similar one). I hope in Schwab API they will introduce client_id which we can pass with create order request, similar to what other brokers do.

[alexgolec]

@studerw Unrelated to this PR, but: can you share more about this latest update from TDA/Schwab about API timing? Is there a full text you can publish?

[studerw]

@alexgolec Take a look at this: June Announcment.

I personally have not got an email with a link to create a Schwab developer API, but I'm on the lookout for it.

[studerw]

update @alexgolec I just looked and I did get a notice for the new developer API at Schwab sent to my old email.

Regardless, I was able to create a new account on their Beta site here: Schwab beta. I don't think you need a special invite code, though not sure how much access I will actually have.

[alexgolec]

I think I received a similar email as well. I went ahead and registered, but I haven't heard anything about my application for API access yet. Have you been able to do anything besides request access?

[studerw]

I did not try anything yet because my account hasn’t even been moved over. I do have another Schwab account, though, so I’ll check it out later this week and report back.