Feat/filters #904

ciaranschutte · 2024-10-11T01:59:30Z

add graphql args to network field, this is a Query top level field and the args can be passed on through to node query creation
uses already established gql query functions to pass gql args as seperate variable (JSON type appears to only work in this manner anyway)
API always has to query aggregations as a subfield of network

…available for node

modules/server/src/network/resolvers/aggregations.ts

joneubank

I think this looks fine but have a question about making sure our variables in the fetch request match the variables we list in the gql query. Perhaps important thing to test.

joneubank · 2024-10-13T01:02:47Z

modules/server/src/network/gql.ts

@@ -33,7 +33,7 @@ export const fetchGql = ({
 }: {
 	url: string;
 	gqlQuery: string;
-	variables?: Record<string, string>;
+	variables?: Record<string, unknown>;


joneubank · 2024-10-13T01:06:07Z

modules/server/src/network/resolvers/aggregations.ts

+	const aggregationsString = !isEmpty(fields)
+		? `aggregations(filters: $filters, aggregations_filter_themselves: $aggregations_filter_themselves, include_missing: $include_missing) ${fields}`
+		: '';
+	const gqlString = `query nodeQuery($filters: JSON, $aggregations_filter_themselves: Boolean, $include_missing: Boolean) {${documentName} { hits { total }  ${aggregationsString} }}`;


we are declaring 3 variables here... if they are not provided in the args object will GQL throw an error?

it's ok for these. default gql args are nullable. there would be an error if args were required using '!' eg. (filter: JSON!)

joneubank · 2024-10-13T01:06:58Z

modules/server/src/network/resolvers/aggregations.ts

 type NetworkQuery = {
 	url: string;
 	gqlQuery: DocumentNode;
+	args: Record<string, unknown>;
 };


Type declared after it is used.

joneubank · 2024-10-13T01:07:30Z

modules/server/src/network/resolvers/aggregations.ts

@@ -55,6 +57,7 @@ const fetchData = async <SuccessType>(
 type NetworkQuery = {
 	url: string;
 	gqlQuery: DocumentNode;
+	args: Record<string, unknown>;


We declare some specific variables in the gql query, should we be requiring these in the args?

We could. I'd love your typescript insight here.

Queries passing args that aren't defined will get an error straight from the gql server. Something like:

"message": "Unknown argument \"random_arg\" on field \"Query.network\".", "extensions": { "code": "GRAPHQL_VALIDATION_FAILED",

So once we trust gql (and we should, strongly typed queries is a benefit it provides), args: Record<string, unknown> could always be args: Record<string, { ...known gql args types... }>

I think it's safe to pass on through the args "as is" because:

We don't do anything with the args right now, they're just passed through as variables.
The query creation is only string manipulation so TS wouldn't provide any benefit there by typing the args.

GQL server will error itself because a type not matching the schema will throw an error

Bigger question is something that impacts a lot of the project - we have an intersection of GQL types and TS types. There's no quick and easy way to do this. The most straightforward we've had so far requires a build step (that's the thing Sam added for platform - I can't recall the library name - some kind of gql-codegen)

Luckily for us, this module only exposes a single function whose interface needs to be typed. The function aggregationPipeline is exported and shouldn't allow any args, it should be specifying what we expect to be set. To that effect, consider introducing the type NetworkAggregationArgs and then update the function declaration to:

type NetworkAggregationArgs = { filters: object; aggregations_filter_themselves: boolean; include_missing: boolean; }; /** * Query each remote connection * * @param queries - Query for each remote connection * @param requestedAggregationFields * @returns */ export const aggregationPipeline = async ( configs: NodeConfig[], requestedAggregationFields: RequestedFieldsMap, args: NetworkAggregationArgs, ) => {

You'll then need to put corresponding updates into the GQL network resolvers to make sure the args provided from the client are properly validated and typed to be passed into the aggregation pipeline.

joneubank · 2024-10-16T16:15:40Z

modules/server/src/network/utils/sqon.ts

+export const isSQONFilter = (filter: unknown) => {
+	// An error will be thrown if the provided input is invalid.
+	SQONBuilder.from(filter);
+};


Shouldn't this return boolean to check the data type? If you want an error thrown at a specific location then throw from there, but this won't be very reusable if it either does nothing or throws.

Also, lets add a TSDocs comment for this exported utility.

the return value is a SQON object, which we don't want, we want to keep it as a string.
So the only purpose this has is to throw an error if it's not a SQON

joneubank · 2024-10-16T16:17:49Z

modules/server/src/network/resolvers/index.ts

+				try {
+					isSQONFilter(args.filters);
+				} catch (err) {
+					throw `SQON filters are not valid. ${JSON.stringify(err)}`;


Does GQL want us to throw strings instead of an error like new Error(message)?

It seems weird that you created isSQONFilter to throw an error and then immediately caught and threw a different error. Would be more reusable to have the sqon type check function return a Result with success containing the SQON and the failure containing the error message you want to include in your new thrown error.

import SQONBuilder, {SQON} from '@overture-stack/sqon-builder'; /** * Attempts to convert a variable to a SQON. A Result is returned, where if successful * the type checked SQON object is returned, and if a failure occurs then the error data * from the SQONBuilder is returned as the failure message. * * @param filter * @returns */ export const convertToSqon = (filter: unknown): Result<SQON, 'INVALID_SQON'> => { try { const output = SQONBuilder.from(filter); return success(output.toValue()); } catch (error: unknown) { let message = `${error}`; // Will convert the error to a string, works fine with the expected ZodError without reuiring importing Zod to arranger return failure('INVALID_SQON', message) } };

With this, you can also update the NetworkAggregationArgs to have filters as a SQON instead of object.

Bonus, I'd rename your file httpResponse.ts to be result.ts.

justincorrigible · 2024-10-18T16:15:57Z

modules/server/src/network/resolvers/aggregations.ts

@@ -20,12 +31,15 @@ import { CONNECTION_STATUS, NetworkNode } from './networkNode';
 const fetchData = async <SuccessType>(
 	query: NetworkQuery,
 ): Promise<Result<SuccessType, typeof CONNECTION_STATUS.error>> => {
-	const { url, gqlQuery } = query;
+	const { url, gqlQuery, queryVariables } = query;
+
 	console.log(`Fetch data starting for ${url}`);


ToDo: consider logger instance/level, so this doesn't come up in regular Arranger instances (unless that may actually be desirable?)

modules/server/src/network/utils/sqon.ts

Co-authored-by: Anders Richardsson <[email protected]>

joneubank

Looks functionally good. Some comments here for small updates, mostly regarding commenting the code.

joneubank · 2024-10-21T14:52:22Z

modules/server/src/network/resolvers/aggregations.ts

@@ -93,18 +102,28 @@ const convertFieldsToString = (requestedFields: RequestedFieldsMap) => {
 	return gqlFieldsString;
 };

+/**
+ * Query string creation


This is not a useful description. Consider expanding to describe what the content of the query string will be and how that string can be used. Also useful, consider: what are the arguments and how are they used?

joneubank · 2024-10-21T14:57:04Z

modules/server/src/network/resolvers/index.ts

+export type NetworkAggregationArgs = {
+	filters?: object;
+	aggregations_filter_themselves?: boolean;
+	include_missing?: boolean;
+};


I think a comment block for this would be good. You have a comment below that says "type should match gql typedefs", but it would be good to directly mention which gql typedef, maybe including the path to that file or at least its name. As a first time reader of this file, I should know that this type is describing the inputs to a specific gql query.

Good idea! I'll include some kind of name link. I will avoid using path because if files are moved there's a string in a comment that needs to be updated.

joneubank · 2024-10-21T14:57:58Z

modules/server/src/network/utils/sqon.ts

+export const convertToSqon = (filter: unknown): Result<SQON, 'INVALID_SQON', void> => {
+	try {
+		const output = SQONBuilder.from(filter);
+		return success(output);
+	} catch (error: unknown) {
+		const message = `${error}`; // Will convert the error to a string, works fine with the expected ZodError without requiring importing Zod to Arranger
+		return failure('INVALID_SQON', message);
+	}
+};


This is working fine, consider using the SQON schema exported with SQONBuilder:

import SQONBuilder, {SQON} from '@overture-stack/sqon-builder'; const isSqon = (input: unknown): input is SQON => SQON.safeParse(input).success;

Ciaran Schutte added 11 commits October 10, 2024 03:41

add TSdoc example

f68c845

add TSdoc comment

c500182

pass more data into Aggs accumulator

b6328cd

add schema map for query time

333161d

rework agg accumulator to add empty agg to bucket if aggregation not …

fbf040e

…available for node

fix test

35ba619

clean up logic to account for only Aggregations type

6edc5aa

account for empty aggs field in node query

6ddaa8c

cleanup

48ec461

disabled network Agg tests

cc01f99

add args to Aggregations gql typedef

ccd518a

ciaranschutte changed the base branch from feat/rearrange_gql_endpoint to feat/aggregate_not_available October 11, 2024 01:59

Ciaran Schutte added 2 commits October 11, 2024 09:58

add args to network field

a6403ec

pass gql resolvers args to query creation

17d490d

ciaranschutte commented Oct 11, 2024

View reviewed changes

modules/server/src/network/resolvers/aggregations.ts Outdated Show resolved Hide resolved

Base automatically changed from feat/aggregate_not_available to feature/federated_search October 12, 2024 05:38

Ciaran Schutte added 3 commits October 12, 2024 20:44

remove console log

7ea5118

add include missing"

d025691

add include missing:

0c67fd6

ciaranschutte marked this pull request as ready for review October 13, 2024 00:51

ciaranschutte requested review from justincorrigible, joneubank and demariadaniel October 13, 2024 00:51

joneubank reviewed Oct 13, 2024

View reviewed changes

ciaranschutte requested a review from joneubank October 13, 2024 01:51

Ciaran Schutte added 3 commits October 16, 2024 03:53

add overture sqon builder dep

2cc52e5

move up type

20f856e

add SQON check and args type

1ed6a86

joneubank reviewed Oct 16, 2024

View reviewed changes

rename file

b45ed0d

Ciaran Schutte added 2 commits October 18, 2024 04:49

check for SQON correctness but pass ordinary object through on success

2de1480

add comment

ad55732

ciaranschutte requested a review from joneubank October 18, 2024 13:13

justincorrigible reviewed Oct 18, 2024

View reviewed changes

modules/server/src/network/utils/sqon.ts Outdated Show resolved Hide resolved

Update modules/server/src/network/utils/sqon.ts

ccb166b

Co-authored-by: Anders Richardsson <[email protected]>

joneubank approved these changes Oct 21, 2024

View reviewed changes

ciaranschutte merged commit 0e03dee into feature/federated_search Oct 21, 2024
0 of 2 checks passed

ciaranschutte deleted the feat/filters branch October 21, 2024 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/filters #904

Feat/filters #904

ciaranschutte commented Oct 11, 2024 •

edited

Loading

joneubank left a comment

joneubank Oct 13, 2024

joneubank Oct 13, 2024

ciaranschutte Oct 13, 2024

joneubank Oct 13, 2024

joneubank Oct 13, 2024

ciaranschutte Oct 13, 2024

joneubank Oct 14, 2024

joneubank Oct 16, 2024

ciaranschutte Oct 16, 2024

joneubank Oct 16, 2024 •

edited

Loading

joneubank Oct 16, 2024

justincorrigible Oct 18, 2024

joneubank left a comment

joneubank Oct 21, 2024

joneubank Oct 21, 2024

ciaranschutte Oct 21, 2024

joneubank Oct 21, 2024

Feat/filters #904

Feat/filters #904

Conversation

ciaranschutte commented Oct 11, 2024 • edited Loading

joneubank 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

joneubank Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joneubank 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

ciaranschutte commented Oct 11, 2024 •

edited

Loading

joneubank Oct 16, 2024 •

edited

Loading