feat(reply headers): rfc, scaffold #416

bludzhd · 2020-04-21T15:56:35Z

No description provided.

AVVS · 2020-04-22T16:39:15Z

packages/core/src/constants.ts

@@ -96,3 +96,5 @@ export const PluginsPriority = [

 export const PLUGIN_STATUS_OK = 'ok'
 export const PLUGIN_STATUS_FAIL = 'fail'
+
+export const kReplyHeaders = Symbol('replyHeaders')


Suggested change

export const kReplyHeaders = Symbol('replyHeaders')

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

AVVS · 2020-04-22T16:40:17Z

packages/core/src/plugins/amqp/router/adapter.ts

 import { Router } from '../../router/factory'
+import { kReplyHeaders } from '@microfleet/transport-amqp/lib/constants'


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

AVVS · 2020-04-22T16:40:59Z

packages/core/src/plugins/amqp/router/adapter.ts

+        .finally(() => {
+          raw.properties[kReplyHeaders] = Object.fromEntries(serviceRequest.getReplyHeaders())
+        });


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

triadium · 2020-11-01T16:42:37Z

packages/core/docs/rfc/service-request.md

+#### `.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.
+


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.

triadium · 2020-11-01T16:45:35Z

packages/core/src/constants.ts

@@ -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')


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.

triadium

To facilitate the tasks of processing the results inside the actions of Microfleet-based services, it is better to separate the interfaces into request / reply and implement lightweight wrapper-classes for managing the reply structure.
Instances will be created using factories bound to routes by request options. In the request options, you can specify the lean attribute, according to which the reply data will be returned as is without creation an instance of the wrapper-class.

sky3d · 2020-11-23T19:05:38Z

packages/core/docs/rfc/service-request.md

+each of the Transports effortlessly.
+
+
+In order to respect backwards compatibility, we should be able to choose whether the reply will contain **only 


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

sky3d · 2020-11-23T19:07:54Z

packages/core/docs/rfc/service-request.md

+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


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

sky3d · 2020-11-23T19:29:16Z

packages/core/src/plugins/amqp/router/adapter.ts

-      const promise = dispatch(actionName, opts)
+      const promise = dispatch(actionName, serviceRequest)
+        .finally(() => {
+          raw.properties[kReplyHeaders] = Object.fromEntries(serviceRequest.getReplyHeaders())


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

sky3d · 2020-11-23T19:42:08Z

packages/core/src/plugins/http/handlers/hapi/router/adapter.ts

 import _require from '../../../../../utils/require'
 import { Router } from '../../../../router/factory'
+import { createServiceRequest} from "./service-request-factory";
+
+const setReplyHeader = (response: ResponseObject) => (value: ReplyHeaderValue, title: string) => {


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())

sky3d · 2020-11-23T19:42:27Z

packages/core/src/plugins/http/handlers/hapi/router/adapter.ts

+      const responseData = await dispatch(actionName, serviceRequest)
+
+      response = h.response(responseData)
+      serviceRequest.getReplyHeaders().forEach(setReplyHeader(response))


See call above

sky3d · 2020-11-23T19:51:40Z

packages/core/src/plugins/socketIO/router/adapter.ts

+
+    if (!callback) return
+
+    const response = options && options.simpleResponse === false


I suggest to get rid of the simpleResponse condition and return an object with scheme: { data, headers } using approach below:

import { isNil } from 'lodash/fp'; const omitNullOrUndefined = omitBy(isNil); // ... callback(err, omitNullOrUndefined(response))

sky3d · 2020-11-23T19:53:16Z

packages/core/test/amqp/helpers/actions/success-set-header.js

+const { ActionTransport } = require('./../../../../src');
+
+function SuccessSetHeaderAction(serviceRequest) {
+  serviceRequest.setReplyHeader('x-your-response-header', 'header value');


Why not use the simplest reply headers declaration ?
serviceRequest.replyHeaders = { 'x-your-response-header' : 'header value' }

sky3d · 2020-11-23T20:07:48Z

packages/core/docs/rfc/service-request.md

+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**.


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

SergeyTolmachev · 2020-12-01T02:58:46Z

packages/core/docs/rfc/service-request.md

+
+
+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 


What do you mean "early headers validation"?

SergeyTolmachev · 2020-12-01T03:03:54Z

packages/core/docs/rfc/service-request.md

+
+
+In order to respect backwards compatibility, we should be able to choose whether the reply will contain **only 
+message body or include headers** as well. This will be resolved with boolean **simpleResponse** option on the Transport


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;

SergeyTolmachev · 2020-12-01T03:17:04Z

packages/core/docs/rfc/service-request.md

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


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;

SergeyTolmachev · 2020-12-01T03:26:03Z

packages/core/docs/rfc/service-request.md

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


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;

SergeyTolmachev · 2020-12-01T03:31:32Z

packages/core/docs/rfc/service-request.md

+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`:


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

I thought that ServiceRequest has to be transport agnostic.

SergeyTolmachev · 2020-12-01T03:46:23Z

packages/core/src/plugins/http/handlers/hapi/router/adapter.ts

+
+const setReplyHeader = (response: ResponseObject) => (value: ReplyHeaderValue, title: string) => {
+  // set-cookie header exceptional case is correctly implemented by hapi
+  return Array.isArray(value)


Can we normalize that and always get an array?

SergeyTolmachev · 2020-12-01T03:53:21Z

packages/core/src/plugins/http/handlers/hapi/router/attach.ts

@@ -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) {


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

SergeyTolmachev · 2020-12-01T03:57:35Z

packages/core/src/plugins/http/handlers/hapi/router/attach.ts

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


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;

feat(reply headers): rfc, scaffold

a6f5537

AVVS reviewed Apr 22, 2020

View reviewed changes

bludzhd added 5 commits April 27, 2020 19:26

feat(reply headers): rfc

c439ca8

fix(reply headers): tests

63d0e25

feat(reply headers): update rfc

8c3a5d2

feat(reply headers): update rfc

0178741

feat(reply headers): update rfc

3d4ff65

triadium reviewed Nov 1, 2020

View reviewed changes

sky3d reviewed Nov 23, 2020

View reviewed changes

SergeyTolmachev reviewed Dec 1, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(reply headers): rfc, scaffold #416

feat(reply headers): rfc, scaffold #416

bludzhd commented Apr 21, 2020

AVVS Apr 22, 2020

AVVS Apr 22, 2020

AVVS Apr 22, 2020

triadium Nov 1, 2020

triadium Nov 1, 2020

triadium left a comment

sky3d Nov 23, 2020

sky3d Nov 23, 2020

sky3d Nov 23, 2020

sky3d Nov 23, 2020

sky3d Nov 23, 2020

sky3d Nov 23, 2020

sky3d Nov 23, 2020

sky3d Nov 23, 2020

SergeyTolmachev Dec 1, 2020

SergeyTolmachev Dec 1, 2020

SergeyTolmachev Dec 1, 2020

SergeyTolmachev Dec 1, 2020

SergeyTolmachev Dec 1, 2020

SergeyTolmachev Dec 1, 2020

SergeyTolmachev Dec 1, 2020

SergeyTolmachev Dec 1, 2020

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

		import { Router } from '../../router/factory'
		import { kReplyHeaders } from '@microfleet/transport-amqp/lib/constants'

		each of the Transports effortlessly.


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


		if (!callback) return

		const response = options && options.simpleResponse === false



		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



		In order to respect backwards compatibility, we should be able to choose whether the reply will contain **only
		message body or include headers as well. This will be resolved with boolean simpleResponse** option on the Transport


		Must normalize title.

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

feat(reply headers): rfc, scaffold #416

Are you sure you want to change the base?

feat(reply headers): rfc, scaffold #416

Conversation

bludzhd commented Apr 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triadium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment