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

feat(reply headers): rfc, scaffold #416

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
228 changes: 228 additions & 0 deletions packages/core/docs/rfc/service-request.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,228 @@
# Service Request

## Overview and Motivation
The Service handles incoming messages via several action Transports. On each incoming message Transport Router Adapter
builds an object with `ServiceRequest` type and passes it to the Dispatcher. There is no common constructor function now.
This object becomes available in the Action Handler as an argument. Its structure is abstract and functionality is
basically transport-agnostic.


Most transport protocols natively have headers in messages. While currently every Transport Router Adapter is able to
parse incoming messages including its optional headers, in order to fully support messaging protocols the Service should
provide a way to set, modify and remove response message headers. That's why we need to implement **common reply headers
API**.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW I would prefer responseHeaders name instead of replyHeaders that is symmetric to Request.



Considering Node V8 hidden classes concept, to minimize hidden class trees number and respectfully maximize the
performance, the Service Request instance must always aim to have same object shape and its properties initialization
order. In order to make it even more performant we have to choose functions over ES6 classes for the Service Request
implementation. Due to that we need to use **single constructor function** to instantiate Service Request objects
anywhere.


Developer experience could be improved in two ways: early headers validation and **strict and common validation policy**.
Early validation will help a developer to avoid errors on transport level. Since the limitations may be different, we

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean "early headers validation"?

must comply with the strictest among all transports validation rules, so that any Action Handler could start to support
each of the Transports effortlessly.


In order to respect backwards compatibility, we should be able to choose whether the reply will contain **only
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest simpleResponse options is not good design. Instead of this option I recommend to use an object with omitting undefined/empty values for 'headers' or 'data'
{ data? , headers? }

Proc - No need to setup additional options. More clean call.
Cons - Breaking change for previous versions.

If backward compatibility is required, it should be handled in a special way in code.
To get the data use the workaround below:
const actualData = response && response.data || response

message body or include headers** as well. This will be resolved with boolean **simpleResponse** option on the Transport

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice shot;
I guess options are more prefferable and clean way to use this service, especially for users who doesn't familliar with this toolkit;

Plugin level, which value must be `true` by default.

## Service Request Interface

### Properties

#### `.transport: TransportTypes`
Transport name that handles the incoming request.

Must be set by Transport Router Adapter.

#### `.method: RequestMethods`
Request method.

Virtual value, which, depending on transport design, should either preserve its original request method name or provide
its custom name.

Must be set by Transport Router Adapter.

#### `.query: object`
Original message may contain query params.

Transport Router Adapter should extract query data and set it as query.

Notice that `.query` value may be possibly modified during the Request step of the Validation Lifecycle: it could be
filtered, assigned by default values and underlying data types could be coerced.

#### `.headers: any`
Original message may contain headers. Transport Router Adapter should extract headers data and set it as params.
The responsibility for extracting request headers and setting it to the Service Request must lay on the
Transport Router Adapter implementation.

#### `.params: any`
Original message may contain params.

Transport Router Adapter should extract request data and set it as params.

Notice that `.params` value may be possibly modified during the Request step of the Validation Lifecycle: it could be
filtered, assigned by default values and underlying data types could be coerced.

#### `.transportRequest?: any`
Third-party request instance.

May be set by Transport Router Adapter.

#### `.log?: { trace(...args: any[]): void; debug(...args: any[]): void; info(...args: any[]): void; warn(...args: any[]): void; error(...args: any[]): void; fatal(...args: any[]): void; }`
Router must set Log child instance with a unique Request identifier.

#### `.socket?: NodeJS.EventEmitter`
In order to provide web sockets protocol support we need to operate on socket instance. It should be set whenever
socket instance is available.

#### `.parentSpan`
When a Tracer is enabled, property may hold a tracer parent span, which context must be supplied by the Transport.

#### `.route: string`
Route name may contain two parts joined by dot - optional Router prefix and required path to the Action Handler,
transformed to dot case. It shall result into the following format:
```
'router-prefix.path.to.the.action.handler'
```
Assuming that the Router plugin prefix configured as `'payments'`, the path to the action
relative to the routes directory defined by Router plugin configuration is `'transactions/create'`, resulting route
value will be `payments.transactions.create`.

*Notice: Route name should be transport-agnostic and therefore must not contain Transport route prefix.*

Route name must be set during the Request step of the Request Lifecycle.

#### `.action: ServiceAction`
When the route match is found, the Router must provide an Action instance to the Service Request.

Action must be set during the Request step of the Request Lifecycle.

#### `.auth: any`
Original message may contain authentication data. Considering the Action authentication strategy it may be resolved
during the Auth step of Request Lifecycle and set as the `.auth` property value.

#### `.span`
When a Tracer is enabled, property must hold a tracer span, initialized as a `.parentSpan` child.

#### `.locals`
By design, this property recommended usage is to share data between Request Lifecycle steps, as well as pass it through
when using Internal Transport. Could be set anywhere during the Request Lifecycle.

#### `.[kReplyHeaders]: Map<string,string|string[]>`
Reply message headers container. Could be set anywhere during the Request Lifecycle with. Should be used to collect and
deliver headers to original reply message.

### Methods
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ServiceRequest interface became to much more complicated. So many methods was added to manage only one headers field. The ServiceRequest interface should be solid and minimilstic.
I suggest not to extract new ServiceRequestInterface, but to extend the existing one with one field:

// type ReplyHeaders: { [key: string]: string } 
responseHeader? : ReplyHeaders


#### `.setReplyHeader(title: string, value: number|string|array<string>): void`
Sets reply header to the map.

Must normalize title.
Must cast numeric value to string.
Must validate title and value. Must throw exception if any of arguments is invalid.

If normalized `title` is `set-cookie`, it's being appended to previously set `set-cookie` value.
Else it`s value is being overridden:

```js
function actionHandler(serviceRequest) {
serviceRequest.setReplyHeader('x-rate-limit', 10000)
serviceRequest.setReplyHeader('x-rate-limit', 20000)

serviceRequest.setReplyHeader('set-cookie', 'state=sg687gjjsdg69847gjsh; Domain=app.com; Secure; HttpOnly')
serviceRequest.setReplyHeader('set-cookie', 'visitor=798; Domain=app.com; Secure; SameSite=Lax')

serviceRequest.setReplyHeader('x-location', 'lat=58.259624, lng=55.919243')
serviceRequest.setReplyHeader('X-LOCATION', 'lat=64.547589, lng=39.758303')
}
```

This will make the reply headers map be like:
```
Map {
'set-cookie' => [
'state=sg687gjjsdg69847gjsh; Domain=app.com; Secure; HttpOnly',
'visitor=798; Domain=app.com; Secure; SameSite=Lax'
],
'x-rate-limit' => '20000',
'x-location' => 'lat=64.547589, lng=39.758303',
}
```
Which will result in two different header lines for HTTP transport.

Comment on lines +122 to +157
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest not to create exception (set-cookie) in the code of the core package, but to leave it up to the userland side.
Let's extend the interface Service Request Interface with the AddReplyHeader method, which will be to form an array of values ​​for the specified header.
The setReplyHeader method will be to replace the entire header value with the specified one without exception.

#### `.removeReplyHeader(title: string): void`
Normalizes title and removes header from headers map.

Usage:
```js
function actionHandler(serviceRequest) {
serviceRequest.removeReplyHeader('x-rate-limit')
serviceRequest.removeReplyHeader('set-cookie')
}
```

#### `.getReplyHeaders(): Map<string,string|string[]>`
Gets all previously initialized headers from headers map.

Usage:
```js
function actionHandler(serviceRequest) {
serviceRequest.removeReplyHeader('x-rate-limit')
serviceRequest.removeReplyHeader('set-cookie')
}
```

#### `.getReplyHeader(title: string): string|string[]`
Gets header from map.

Must normalize title.

### `.isValidReplyHeader(title: string, value: string|string[]): boolean`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about meaning of ### but I think it is another one method in ServiceRequest;
Is it a private method? Or user has to use it manually?
I guess we need to discuss about process of headers validation and describe it in RFC; Does it omit header if it is not valid or throw an exception, etc...
I prefer just omit not valid header but it could be implicit;

Must validate title and value.
Validation rules are:
* Is ASCII
Empty strings are allowed.

## Implementation design

### AMQP Transport
Extract headers collection and set it under `kReplyHeaders` key of `raw.properties`, where `kReplyHeaders` is a Symbol
defined by `@microfleet/transport-amqp` library.

### HTTP Transport
Consider HTTP RFC relative to [`set-cookie` header](https://tools.ietf.org/html/rfc7230#section-3.2.2).

#### Hapi Handler
Extract headers collection and set it to the response using Hapi Response Toolkit.

### SocketIO Transport
Expect new optional argument `options` to be passed on `.emit`:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use different properties to save and handle headers depends on transport layer?

I thought that ServiceRequest has to be transport agnostic.

```
.emit(action: string, body: any, options: SocketIOMessageOptions, callback: RequestCallback): void
```

When present, options may contain `simpleResponse` setting.
When `simpleResponse` option value is `false`, callback `result` must be resolved with `data` containing reply message,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it has to work vice versa when we talk about backward compatibility. It there is no simpleResponse => then send old format message (just body) and when we got an option => we send new format message;
Probably, we have to change an option name;

and `headers` containing headers. Otherwise, callback `result` must be resolved with reply message.

```
{ headers: { [key: string]: string }, data?: unknown }
```

Usage
```
client.emit('echo', { message: 'ok' }, { simpleResponse: false }, (error, result) => {
const { data, headers } = result;
});
```

### Internal Transport
Router plugin exposes two `dispatch` methods to the Service: `.dispatch()` and `.router.dispatch()`.
Extend Router plugin `.dispatch` method signature with optional `options` argument to pass `simpleResponse` flag.
When `simpleResponse` option value is `false`, callback `result` must be resolved with `data` containing reply message,
and `headers` containing headers. Otherwise, callback `result` must be resolved with reply message.
2 changes: 2 additions & 0 deletions packages/core/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,5 @@ export const PluginsPriority = [

export const PLUGIN_STATUS_OK = 'ok'
export const PLUGIN_STATUS_FAIL = 'fail'

export const kReplyHeaders = Symbol.for('microfleet:replyHeaders')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To achieve independence from transport packages, it is better to declare the symbol in the core package, and use Symbol.for (...) in the other packages as needed.

27 changes: 7 additions & 20 deletions packages/core/src/plugins/amqp/router/adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ import get = require('get-value')
import is = require('is')
import noop = require('lodash/noop')
import { ActionTransport } from '../../..'
import { ServiceRequest } from '../../../types'
import { Router } from '../../router/factory'
import { kReplyHeaders } from '@microfleet/transport-amqp/lib/constants'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets update @microfleet/transport-amqp and add symbol as exports
something like symbols.replyHeaders

import { createServiceRequest } from './service-request-factory'

// cached var
const { amqp } = ActionTransport
Expand Down Expand Up @@ -41,28 +42,14 @@ function getAMQPRouterAdapter(router: Router, config: any) {
const routingKey = properties.headers['routing-key'] || properties.routingKey
const actionName = normalizeActionName(routingKey)

const opts: ServiceRequest = {
// initiate action to ensure that we have prepared proto fo the object
// input params
// make sure we standardize the request
// to provide similar interfaces
params,
action: noop as any,
headers: properties,
locals: Object.create(null),
log: console as any,
method: amqp as ServiceRequest['method'],
parentSpan: raw.span,
query: Object.create(null),
route: '',
span: undefined,
transport: amqp,
transportRequest: Object.create(null),
}
const serviceRequest = createServiceRequest(properties, params, raw.span);

increaseCounter()
try {
const promise = dispatch(actionName, opts)
const promise = dispatch(actionName, serviceRequest)
.finally(() => {
raw.properties[kReplyHeaders] = Object.fromEntries(serviceRequest.getReplyHeaders())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor it! Use more descriptive method name for these operations.
Or use adapter specific utility functions to setup transport message props based of request.responseHeaders

});
Comment on lines +50 to +52
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we put this into finally handle of try/catch/finally ?

const response = await wrapDispatch(promise, actionName, raw)
setImmediate(next, null, response)
} catch (e) {
Expand Down
19 changes: 19 additions & 0 deletions packages/core/src/plugins/amqp/router/service-request-factory.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { ActionTransport, ServiceRequestInterface } from '../../..';
import { ServiceRequest } from '../../../utils/service-request';

export const createServiceRequest = (
properties: any,
params: any,
parentSpan: any
): ServiceRequestInterface => (
new ServiceRequest(
ActionTransport.amqp,
'amqp',
Object.create(null),
properties,
params,
Object.create(null),
undefined,
parentSpan,
)
);
10 changes: 5 additions & 5 deletions packages/core/src/plugins/http/handlers/hapi/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,12 @@ function createHapiServer(config: any, service: Microfleet): PluginInterface {
}

const registrations = []
for (const pluguinConfiguration of plugins) {
for (const pluginConfiguration of plugins) {
registrations.push({
options: pluguinConfiguration.options,
plugin: typeof pluguinConfiguration.plugin === 'string'
? require(pluguinConfiguration.plugin)
: pluguinConfiguration.plugin,
options: pluginConfiguration.options,
plugin: typeof pluginConfiguration.plugin === 'string'
? require(pluginConfiguration.plugin)
: pluginConfiguration.plugin,
})
}

Expand Down
43 changes: 18 additions & 25 deletions packages/core/src/plugins/http/handlers/hapi/router/adapter.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
import { HttpStatusError } from '@microfleet/validation'
import Bluebird = require('bluebird')
import Errors = require('common-errors')
import { Request } from '@hapi/hapi'
import noop = require('lodash/noop')
import { Request, ResponseObject, ResponseToolkit } from '@hapi/hapi'
import { FORMAT_HTTP_HEADERS } from 'opentracing'
import { ActionTransport, Microfleet } from '../../../../..'
import { ServiceRequest, RequestMethods } from '../../../../../types'
import { Microfleet, ReplyHeaderValue } from '../../../../..'
import _require from '../../../../../utils/require'
import { Router } from '../../../../router/factory'
import { createServiceRequest} from "./service-request-factory";

const setReplyHeader = (response: ResponseObject) => (value: ReplyHeaderValue, title: string) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to use FP here. Something like that:

import { map } from 'lodash/fp'

const setHeaders = (response) => (key, val) => response.header(key, val)
const setResponseHeaders = setHeaders(response)

const setReplyHeaders = items => map((x) => setResponseHeaders(x))

setReplyHeaders(serviceRequest.getReplyHeaders())

// set-cookie header exceptional case is correctly implemented by hapi
return Array.isArray(value)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we normalize that and always get an array?

? value.forEach(item => response.header(title, item))
: response.header(title, value)
}

export default function getHapiAdapter(actionName: string, service: Microfleet) {
const Boom = _require('@hapi/boom')
Expand Down Expand Up @@ -61,36 +67,23 @@ export default function getHapiAdapter(actionName: string, service: Microfleet)
// pre-wrap the function so that we do not need to actually do fromNode(next)
const dispatch = Bluebird.promisify(router.dispatch, { context: router })

return async function handler(request: Request) {
return async function handler(request: Request, h: ResponseToolkit) {
const { headers } = request

let parentSpan
if (service.tracer !== undefined) {
parentSpan = service.tracer.extract(headers, FORMAT_HTTP_HEADERS)
}

const serviceRequest: ServiceRequest = {
// defaults for consistent object map
// opentracing
// set to console
// transport type
headers,
parentSpan,
action: noop as any,
locals: Object.create(null),
log: console as any,
method: request.method.toLowerCase() as RequestMethods,
params: request.payload,
query: request.query,
route: '',
span: undefined,
transport: ActionTransport.http,
transportRequest: request,
}
const serviceRequest = createServiceRequest(request, parentSpan);

let response: ResponseObject

let response
try {
response = await dispatch(actionName, serviceRequest)
const responseData = await dispatch(actionName, serviceRequest)

response = h.response(responseData)
serviceRequest.getReplyHeaders().forEach(setReplyHeader(response))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See call above

} catch (e) {
response = reformatError(e)
}
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/plugins/http/handlers/hapi/router/attach.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ export default function attachRouter(service: Microfleet, config: any): HapiPlug
server.route({
method: ['GET', 'POST'],
path: '/{any*}',
async handler(request: Request) {
async handler(request: Request, h: ResponseToolkit) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't used hapi. Is it common practise to use letters as arguments of functions? Especially in TS;

const actionName = fromPathToName(request.path, config.prefix)
const handler = hapiRouterAdapter(actionName, service)
return handler(request)
return handler(request, h)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it is prefferable to use one style in code. And I prefer this one instead this:
serviceRequest.getReplyHeaders().forEach(setReplyHeader(response))

And I recommend to use another word to describe variable handler. Probably you can call it hapiHandler. It is unintuitive when function name and variable inside are same;

},
})

Expand Down
Loading