make token options retrieval asynchronous - #74
Conversation
Duslerke
left a comment
Duslerke
left a comment
There was a problem hiding this comment.
Ok, considering this PR is about async, the architecture violation can be left as is.
So for changes that need to be addressing - the Async method name suffixes need to be dropped.
Meanwhile the architecture violation should be addressed in a follow up PR in the spirit of keeping PRs small and to the point.
| [ProducesResponseType(typeof(TokenOptionsResponse), StatusCodes.Status200OK)] | ||
| [HttpGet("options")] | ||
| public IActionResult GetTokenOptions() | ||
| public async Task<IActionResult> GetTokenOptionsAsync() |
There was a problem hiding this comment.
Since we're not a Nuget package developer that needs to maintain Sync and Async versions of exposed methods, there's no value in suffixing our method names with Async.
It only makes code harder to read, meanwhile the async nature is obvious from the interface definition (Task return type), and async declaration within method signatures of implementation. And when it comes to actual method calls, we have await indicating async'ness.
| int GenerateToken(TokenRequestObject tokenRequestObject); | ||
| int? UpdateToken(int tokenId, bool enabled); | ||
| TokenOptionsResponse GetTokenOptions(); | ||
| Task<TokenOptionsResponse> GetTokenOptionsAsync(); |
There was a problem hiding this comment.
As mentioned in above comment, not need for Async suffix.
| } | ||
|
|
||
| public TokenOptionsResponse GetTokenOptions() | ||
| public async Task<TokenOptionsResponse> GetTokenOptionsAsync() |
There was a problem hiding this comment.
As described in above comments, the Async method name suffix is not needed.
| public Task<TokenOptionsResponse> ExecuteAsync() | ||
| { | ||
| return _gateway.GetTokenOptions(); | ||
| return _gateway.GetTokenOptionsAsync(); |
There was a problem hiding this comment.
Oh... just noticed this while I was looking at the lack of await on the gateway call.
You're using the using TokenAdministrationApi.V1.Boundary.Response; model within use case and the gateway.
That's a Hackney architecture no violation. In doing this loose coupling through the response (presentation layer) model, the presentation, domain, and the entity layers get mangled together.
Even when models stay the same between the layers, based on our architecture they should still be separated out as separate classes. So you should have the TokenOptionsEntity that the gateway uses for its internal logic. Then before returning that to the usecase, your GW would map this model to TokenOptions (domain implied) domain model (use case should only ever be aware of domain level models). Then the use case returns the domain model to the controller (presentation layer). Lastly, the controller, maps the domain object to presentation layer TokenOptionsResponse model that it returns as a response to the consumer (as in the model's name).
But this violation can be addressed in the follow up PR.
There was a problem hiding this comment.
Also Async suffixes need to be dropped here and corresponding interface as well.
| } | ||
|
|
||
| public TokenOptionsResponse Execute() | ||
| public Task<TokenOptionsResponse> ExecuteAsync() |
There was a problem hiding this comment.
Also this is not async as it does not perform any async logic, nor the method is marked as async. What this actually does is synchronously return the async gateway call's Task. To make this async, you need to await the GW call.
2 participants
Summary
Updates the token-options endpoint to use asynchronous database queries, following the project's usual async endpoint approach.
Changes
GET /api/v1/tokens/optionsasynchronous.ToListAsync()for the token-option queries.GET /api/v1/tokensendpoint unchanged.Testing