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

JQ filtering in client #20

Merged
merged 9 commits into from
Oct 27, 2023
Merged

JQ filtering in client #20

merged 9 commits into from
Oct 27, 2023

Conversation

gergas3
Copy link
Contributor

@gergas3 gergas3 commented Oct 25, 2023

Add ability to specify a jq filter that is applied to the response body. Allows extracting only parts of the response, which is useful when the response is verbose and the server is only interested in specific part of the response. An example use case is listing Kubernetes resources where the metadata of each item in the response is very verbose. (See test case for a concrete example.) Uses the node-jq library.
The jq tranform expression is passed in the braekhus-jq-response-transform header - not prefixed x- as recommended by https://specs.openstack.org/openstack/api-wg/guidelines/headers.html

Also:

  • add prettier.config.js to codify import order
  • fix husky path and add husky config (yarn prepare failed locally because of cd-ing out of current directory)
  • filter out sensitive authorization header from nested request object inside response object in debug logs

@gergas3 gergas3 marked this pull request as ready for review October 25, 2023 18:41
client/filter.ts Outdated
// jq-node doesn't support `import` syntax
const jq = require("node-jq");

export const jpFilter = async (
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you meant

Suggested change
export const jpFilter = async (
export const jqFilter = async (

client/filter.ts Outdated

const logger = createLogger({ name: "filter" });

// jq-node doesn't support `import` syntax
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm I think rather our node config doesn't support import of this module. Can you explain the error you get here, and leave a TODO to fix our node config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Module '"node-jq"' has no exported member 'jq'.

data: any,
jqHeader: string | string[] | undefined
): Promise<any> => {
const query = isArray(jqHeader) ? jqHeader[0] : jqHeader;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't you want to run this multiple times if there are multiple headers?

Suggested change
const query = isArray(jqHeader) ? jqHeader[0] : jqHeader;
const query = isArray(jqHeader) ? jqHeader : [jqHeader];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean chain the header jq commands? The output of the first is the input of the second?
I don't know if that's helpful, I think chains should be expressible in one query.

client/filter.ts Outdated
return data;
}
try {
// jq.run() returns a Promise
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this comment is unnecessary. The fact the line below passes eslint already means the function returns a promise.

@@ -22,6 +22,7 @@ export const createLogger = <T extends LoggerOptions>(
// Redact the authorization header that may contain a secret token
redact: [
"response.config.headers.authorization", // Axios intercepted response object
"response.request.headers.authorization", // Axios intercepted response object contains the request as well
"request.headers.authorization", // Axios intercepted request object + forwarded request object (JSON RPC request)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you also want just headers? it looks like you manually assign request / response headers to a bare headers field in some logs.

types/index.ts Outdated
@@ -2,6 +2,8 @@ import { Request } from "express";
import core from "express-serve-static-core";
import { IncomingHttpHeaders } from "node:http";

export const JQ_HEADER = "braekhus-jq-response-filter";
Copy link
Contributor

Choose a reason for hiding this comment

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

what motivated you not to add a x- prefix? Any specific reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

types/index.ts Outdated
@@ -2,6 +2,8 @@ import { Request } from "express";
import core from "express-serve-static-core";
import { IncomingHttpHeaders } from "node:http";

export const JQ_HEADER = "braekhus-jq-response-filter";
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a jq program, not a filter, right? Were it a filter I presume your invocation would look more like jq.run(". | " + filter, ...).

Should this be

Suggested change
export const JQ_HEADER = "braekhus-jq-response-filter";
export const JQ_HEADER = "x-braekhus-response-jq-program";

?

Copy link
Contributor Author

@gergas3 gergas3 Oct 27, 2023

Choose a reason for hiding this comment

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

Called it braekhus-response-jq-transform

@gergas3 gergas3 merged commit 7f1971c into main Oct 27, 2023
1 check passed
@gergas3 gergas3 deleted the jq-filtering branch October 27, 2023 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants