From 7f675ecde16d08c85e39ac7c3b9a5049d82d4022 Mon Sep 17 00:00:00 2001 From: andrea rota Date: Mon, 10 May 2021 16:38:43 +0100 Subject: [PATCH 1/6] add WIP API development workflow proposal doc --- docs/README_API_development_workflow.md | 84 +++++++++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 docs/README_API_development_workflow.md diff --git a/docs/README_API_development_workflow.md b/docs/README_API_development_workflow.md new file mode 100644 index 0000000000..7954e5aedc --- /dev/null +++ b/docs/README_API_development_workflow.md @@ -0,0 +1,84 @@ +# API - development workflow + +This document outlines a proposed general workflow for the development of +features in the API and GeoProcessing services. + +As the development team scales out, it is desirable to follow (flexibly) a +workflow that allows to optimize clarity and parallel work. + +## Checklist + +### Alignment on requirements for a component + +* are all the project team members aware of the user-value of the component? +* are there any assumptions implicit in the design that need clarification or + adjustment (because of product needs or because of technical or other + limitations)? + +### definition of the API interface(s) + +* which "resources" (in a REST meaning) need to be handled? +* which endpoints are needed? + * should any *existing* endpoint be changed? + * are *new* endpoints needed? + * should the new endpoints share part of the URL path with existing + endpoints? (this is partly related to a section below on fronting + implementation details with a simpler public interface) +* at this stage we should create all the relevant DTOs for create, update and + get payloads (requests and responses), as well as all relevant types +* if relevant, Swagger documentation may be added now (or after the stub + implementation, see below) + +At this stage we should have full typing as well as scaffolded endpoints. These +would do little by now, except validating payload shape. + +### API e2e tests + +* we should aim to add at least happy-path e2e tests as one of the first + implementation steps +* this means that we should create e2e tests that mimic real requests the API + may receive from API clients, starting from the frontend app +* e2e tests should use DTOs and types from the previous step + +At this stage, we should be able to run tests for the new component and see them +fail. + +### Stub implementation + +* consider whether a CRUD implementation is sufficient for the task +* if so, and if queries require JSON:API query features (filtering, sorting, + sparse fieldsets, includes, pagination), we should base a new service on + `AppBaseService` +* if the CQRS model may be more appropriate, there's still an option to go + very lightweight on this by only keeping command and query DTOs fully distinct +* if CQRS proper is appropriate, it may be the way to go +* at this stage, we should aim for a breakdown of service functions that + maximises the ability to meaningfully unit-test most functions (avoiding + mixing computation and side effects, for example) + +### Documentation + +Add Swagger documentation where relevant, if not done before. + +### Unit tests + +These may be added before or after the actual implementation, as preferred. + +### Actual implementation + +Time to fill in the blanks + +* actual db queries +* interaction with job queues +* computations/validations/etc. + +It will probably be sensible to break down the implementation itself across +distinct PRs especially for larger features or where developers can focus on +distinct portions in parallel. + +At this stage, we should see all tests pass. + +### Edge cases + +* add tests (e2e or unit as relevant) for edge cases that may emerge through + use of the API, or for any bugs that we fix, to avoid regressions. From cc15ea22f3e03433291c633267e76977e7d96bb7 Mon Sep 17 00:00:00 2001 From: andrea rota Date: Tue, 11 May 2021 09:55:41 +0100 Subject: [PATCH 2/6] add notes on agile workflow --- docs/README_API_development_workflow.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/docs/README_API_development_workflow.md b/docs/README_API_development_workflow.md index 7954e5aedc..0a61437231 100644 --- a/docs/README_API_development_workflow.md +++ b/docs/README_API_development_workflow.md @@ -82,3 +82,28 @@ At this stage, we should see all tests pass. * add tests (e2e or unit as relevant) for edge cases that may emerge through use of the API, or for any bugs that we fix, to avoid regressions. + +## Agile workflow + +Please see the general [Vizzuality agile +framework](https://vizzuality.github.io/playbook/projects/agile-framework/) for +reference. + +Additionally: + +* we have a Jira-GitHub integration set up, so please make sure to use + `MARXAN-NNN` labels in branch names and in PR titles; to keep track of where + small items of work fit within larger stories, we should aim to always link + branches and PRs to Jira stories/tasks + +* if splitting up a story or task into smaller items, consider if it is + meaningful to split the corresponding Jira story/task into subtasks; for very + small subtasks this may be overkill, in which case please link branches/PRs to + the main Jira story/task + +* to try to keep cognitive overhead low for all the team members while + collaborating on tasks/reviews, if the subtasks needed for a story/task are + more than what could be easily mapped mentally by a drive-by colleague, please + try to describe in the main Jira story how the pieces fit together, and any + dependencies/blockers, either via a plain list with checkboxes, or through a + sketch From 5e0d2b374dd38e13234d6130e63dcfdd1ed0f804 Mon Sep 17 00:00:00 2001 From: andrea rota Date: Tue, 11 May 2021 18:46:58 +0100 Subject: [PATCH 3/6] WIP definition of e2e test boundaries --- docs/README_API_development_workflow.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/docs/README_API_development_workflow.md b/docs/README_API_development_workflow.md index 0a61437231..e1e0248b5c 100644 --- a/docs/README_API_development_workflow.md +++ b/docs/README_API_development_workflow.md @@ -39,6 +39,26 @@ would do little by now, except validating payload shape. * this means that we should create e2e tests that mimic real requests the API may receive from API clients, starting from the frontend app * e2e tests should use DTOs and types from the previous step +* e2e tests should cover the space between API consumers/clients and the API + itself: we should test up to and including this boundary but not beyond; for + example, we should not test that Redis or BullMQ work as intended, as they are + covered (or should be) by their own tests upstream +* the API boundary in this context includes the GeoProcessing service, as the + actual API service only acts as a simple API gateway towards the GeoProcessing + service: that some requests are handled in practice by the GeoProcessing + service should be considered an implementation detail that API consumers + should not be concerned with +* requests whose processing extends beyond the API/GeoProcessing boundary should + only be tested up to the point of dispatching requests to external systems + (whether via API calls, triggering jobs via message queues, etc.) +* for example, if e2e-testing a request that involves an asynchronous geospatial + calculation handled via BullMQ workers, we should make sure requests from API + clients end up adding the appropriate job to the relevant queue +* we should obviously still test that given relevant job descriptions, an + asynchronous worker will try to perform the associated task (or reject the + job request, if this is incomplete, malformed, cannot be authorized, etc.), + but this may be covered by appropriate unit tests in the GeoProcessing + service itself. At this stage, we should be able to run tests for the new component and see them fail. From 10ab6ecfab4530556513a99e09cd9a4e3b323b80 Mon Sep 17 00:00:00 2001 From: andrea rota Date: Tue, 11 May 2021 18:47:40 +0100 Subject: [PATCH 4/6] =?UTF-8?q?add=20suggestion=20to=20rely=20on=20fa?= =?UTF-8?q?=C3=A7ade=20pattern=20where=20relevant?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/README_API_development_workflow.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/README_API_development_workflow.md b/docs/README_API_development_workflow.md index e1e0248b5c..a70ab3ae4f 100644 --- a/docs/README_API_development_workflow.md +++ b/docs/README_API_development_workflow.md @@ -69,6 +69,11 @@ fail. * if so, and if queries require JSON:API query features (filtering, sorting, sparse fieldsets, includes, pagination), we should base a new service on `AppBaseService` +* if the component needs to interact with distinct services to fulfill its + duties but this is an implementation detail that clients should not be + concerned with (for example, if it needs to consult different data sources and + compute results for API clients from these sources combined), we should + consider a façade pattern * if the CQRS model may be more appropriate, there's still an option to go very lightweight on this by only keeping command and query DTOs fully distinct * if CQRS proper is appropriate, it may be the way to go From 069e4fc93289542891cc02b85a24e202d3d580f8 Mon Sep 17 00:00:00 2001 From: andrea rota Date: Tue, 11 May 2021 19:01:13 +0100 Subject: [PATCH 5/6] add section on internal interfaces --- docs/README_API_development_workflow.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/README_API_development_workflow.md b/docs/README_API_development_workflow.md index a70ab3ae4f..01920aef07 100644 --- a/docs/README_API_development_workflow.md +++ b/docs/README_API_development_workflow.md @@ -81,6 +81,14 @@ fail. maximises the ability to meaningfully unit-test most functions (avoiding mixing computation and side effects, for example) +### Internal interfaces + +* where the structure of the stub implementation (previous section) includes + interfacing between different modules/services by passing messages or commands + (e.g. via BullMQ job queues), we should define the contracts between these + modules/services (for example, message payloads, etc.) as well as mapping out + the flow of commands and data between them + ### Documentation Add Swagger documentation where relevant, if not done before. From b4ad0f17842e791656a6fdbcc2a7ccc814c51621 Mon Sep 17 00:00:00 2001 From: andrea rota Date: Wed, 12 May 2021 12:04:51 +0100 Subject: [PATCH 6/6] attempt to clarify purpose of two sections --- docs/README_API_development_workflow.md | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/docs/README_API_development_workflow.md b/docs/README_API_development_workflow.md index 01920aef07..e0b8c4d846 100644 --- a/docs/README_API_development_workflow.md +++ b/docs/README_API_development_workflow.md @@ -77,9 +77,14 @@ fail. * if the CQRS model may be more appropriate, there's still an option to go very lightweight on this by only keeping command and query DTOs fully distinct * if CQRS proper is appropriate, it may be the way to go -* at this stage, we should aim for a breakdown of service functions that - maximises the ability to meaningfully unit-test most functions (avoiding - mixing computation and side effects, for example) +* irrespective of architectural choices, we should aim for a breakdown of + service functions that maximises the ability to meaningfully unit-test most + functions (avoiding mixing computation and side effects, for example) + +At this stage we should have a stub implementation that could be reviewed for +feedback on the proposed architecture, and with front components (such as +controller handlers and job queues) able to receive "commands" (HTTP requests, +jobs) although there will be no code to process these commands. ### Internal interfaces @@ -89,6 +94,10 @@ fail. modules/services (for example, message payloads, etc.) as well as mapping out the flow of commands and data between them +At this stage, we should be able to review for feedback all the interfaces +between internal components, making sure that interfaces are suitable and +complete, that they don't include unnecessary or unclear data. + ### Documentation Add Swagger documentation where relevant, if not done before.