Implement changes to Cursor API to include the stream transaction id …

[DiscJockeyDJ]

…to the request headers.

fix #322

[DiscoPYF]

The current implementation looks good.

A transactionId parameter has been added to DeleteCursorAsync, PutCursorAsync and PostCursorBody, but I wonder if we could instead add an additional general parameter to the API methods for supporting headers, like we do for query/body parameters? e.g. PostCursorAsync<T>(PostCursorBody postCursorBody, PostCursorHeaders postCursorHeaders), or something a bit more generic.

https://www.arangodb.com/docs/stable/http/transaction-stream-transaction.html

To use the transaction for a supported operation a client needs to specify the transaction identifier in the x-arango-trx-id header on each request.

I think it's worth taking this opportunity to model something, like an interface containing common headers.

@rossmills99 @modios Any thoughts?

There are headers that are applied widely, e.g. x-arango-async: true, but I'm not sure it's a strong enough reason to add support for them on each endpoint, as they could also be implemented differently.

[rossmills99]

I notice from the docs that the transaction ID header is only relevant in the "create cursor" endpoint, so our DeleteCursor or PutCursor APIs should not be affected. Those changes should be removed from the PR.

To address @DiscoPYF 's questions:

I wonder if we could instead add an additional general parameter to the API methods for supporting headers, like we do for query/body parameters? e.g. PostCursorAsync<T>(PostCursorBody postCursorBody, PostCursorHeaders postCursorHeaders), or something a bit more generic.

I think that there are two PostCursorAsync methods. One of them is a convenience method that provides some of the options are method arguments, instead of requiring the PostCursorBody object. I think that it would make sense to include string transactionId = null as an optional argument in the convenience method, and then for the other method introduce a new type that allows additional headers more generically in the request. At least I think that would be consistent with how we've approached things so far. What do you think @DiscoPYF ?

[DiscoPYF]

I think that it would make sense to include string transactionId = null as an optional argument in the convenience method, and then for the other method introduce a new type that allows additional headers more generically in the request.

I agree, this is a good approach (imo) and consistent with our API, indeed.

Not sure how generic the parameter for the headers should be. Example: WebHeaderCollection vs a model with common header properties.

[DiscJockeyDJ]

DeleteCursor or PutCursor APIs should not be affected

Cool 👍 will revert the changes.

then for the other method introduce a new type that allows additional headers more generically in the request

Example: WebHeaderCollection vs a model with common header properties.

Instead of creating a new TransactionId property in PostCursorBody, should a new model be created? Is that correct?

[DiscoPYF]

Yes, correct. I think the idea is to have the following:

public virtual async Task<CursorResponse<T>> PostCursorAsync<T>(
            PostCursorBody postCursorBody, AClassWithHeaderProperties headers)

[DiscJockeyDJ]

PR updated addressing these comments.

DeleteCursor or PutCursor APIs should not be affected.

Instead of creating a new TransactionId property in PostCursorBody, should a new model be created? Is that correct?
Yes, correct

[rossmills99]

Thanks for the update. It appears ConfigureAwait was removed from a lot of the async calls. Can you replace it please? This was added for reasons described in #215

[DiscJockeyDJ]

PR updated.

Reinstated the missed out ConfigureAwait to the endpoints.

[DiscoPYF]

@DiscJockeyDJ Thank you for the update. This implementation fits well with the current APIs. I left minor comments about naming and placing of new methods/classes, but this could be adjusted in an upcoming pull request.

I noticed that some test runs are failing. I will take a look, but approved pending failure resolved.

[DiscoPYF]

It would also be useful to add a test to verify that header properties are being passed on to transport correctly. Example: PostCursorAsync_ShouldUseHeaderProperties can call PostCursorAsync and mock IApiClientTransport to check that the provided WebHeaderCollection contains the expected header key/values. Similar to what PostCollectionAsync_ShouldUseQueryParameter does.

[DiscoPYF]

I ran again the failed test runs and they now pass.

I am listing the failures for future reference:

• https://app.circleci.com/pipelines/github/Actify-Inc/arangodb-net-standard/614/workflows/54ac61ec-5caa-41c0-bb96-32aebf07aa25/jobs/1298

System.Net.Sockets.SocketException : Connection refused

• https://app.circleci.com/pipelines/github/Actify-Inc/arangodb-net-standard/614/workflows/54ac61ec-5caa-41c0-bb96-32aebf07aa25/jobs/1301

ArangoDBNetStandard.ApiErrorException : service unavailable due to startup or maintenance mode

[DiscJockeyDJ]

PR updated to address the above mentioned suggestions 👍

If this class is only for the cursor API, maybe we should name it more explicitely? e.g. CursorHeaderProperties

I am thinking it would be useful to have it protected virtual

It would also be useful to add a test to verify that header properties are being passed on to transport correctly. Example: PostCursorAsync_ShouldUseHeaderProperties

[DiscoPYF]

Thank you @DiscJockeyDJ , looks good to me. Approved 👍

[rossmills99]

Looks good to me