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

Issue 4359 pretty print response #7634

Merged
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
3edcbbc
Issue-4359: Updating to expose a function to stringify the graphQL re…
dfperry5 Jul 1, 2023
e7e726c
Issue-4359: Testing stringify result parameter
dfperry5 Jul 8, 2023
bfb63d3
Issue-4359: Cleaning up tests (removing .only)
dfperry5 Jul 8, 2023
271a07b
Merge branch 'apollographql:main' into Issue-4359-Pretty-Print-Response
dfperry5 Jul 8, 2023
9770a5c
Issue-4359: Removing commented out code in test
dfperry5 Jul 8, 2023
e043d27
Merge branch 'Issue-4359-Pretty-Print-Response' of github.com:dfperry…
dfperry5 Jul 8, 2023
eba009c
Issue-4359: Updating to have appropiate linting
dfperry5 Jul 8, 2023
29bc192
Issue-4359: Removing accidental typo
dfperry5 Jul 8, 2023
59a8e4c
Issue-4359: Updating to be in alignment with Prettier configs
dfperry5 Jul 8, 2023
2408db5
Issue-4359: adding in test cases
dfperry5 Jul 10, 2023
66bf173
Issue-4359: Adding in changeset
dfperry5 Jul 10, 2023
48b1d5b
Issue-4359: Updating snapshots
dfperry5 Jul 10, 2023
10b44be
Issue-4359: Removed Deprecated snapshots
dfperry5 Jul 10, 2023
9ee1620
Issue-4359: Updating with prettier
dfperry5 Jul 10, 2023
4049e9c
Issue-4359: Removing integration tests and associated snapstops, upda…
dfperry5 Jul 10, 2023
3bc750c
Issue-4359: Updating test case to be simpler and easy to read
dfperry5 Jul 10, 2023
31b688c
Issue-4359: Updating to have better naming of the test case, and remo…
dfperry5 Jul 10, 2023
bcbca2d
Issue-4359: Removed stray semi-colon in changeset file
dfperry5 Jul 10, 2023
714c94d
Issue-4359: Defaulting stringifyResult to prettyJSONStringify
dfperry5 Jul 11, 2023
337b432
Issue-4359: Call-site cleanup, and prettier
dfperry5 Jul 11, 2023
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
26 changes: 26 additions & 0 deletions packages/integration-testsuite/src/httpServerTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
GraphQLSchema,
GraphQLString,
type ValidationContext,
type FormattedExecutionResult,
} from 'graphql';
import gql from 'graphql-tag';
import {
Expand Down Expand Up @@ -2060,6 +2061,31 @@ export function defineIntegrationTestSuiteHttpServerTests(
expect(res.body.errors[0].message).toEqual(expected);
});
});

it('uses stringifyResult parameter', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

We can just remove this test altogether with the existence of the other test. It looks like there are a handful of snapshot files that were generated due to using toMatchSnapshot instead of toMatchInlineSnapshot - we can delete those too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on it.

const app = await createApp({
schema,
stringifyResult: (value: FormattedExecutionResult) => {
let result = JSON.stringify(value, null, 1); // stringify, with line-breaks and indents
result = result.replace('it works', 'stringifyResults works!'); // replace text with something custom
return result;
},
});
const expected = {
testString: 'stringifyResults works!',
};
const query = {
query: 'query test{ testString }',
};
const req = request(app)
.get('/')
.set('apollo-require-preflight', 't')
.query(query);
return req.then((res) => {
expect(res.status).toEqual(200);
expect(res.body.data).toEqual(expected);
});
});
Copy link
Member

Choose a reason for hiding this comment

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

I think this is specific to Apollo Server and doesn't necessarily need to be tested for all integrations. I think this might belong in ApolloServer.test.ts instead under a describe('configuration options', ...) block.

There seems to be a couple layers in testing this. Your replace is clearly working as expected but the additional JSON.stringify formatting arguments are of no consequence to this test (i.e. I can remove them and the test still passes) so I'd rather see a test that validates the actual string output (probably with toMatchInlineSnapshot) or something similar.

Sidenote: res.body.data is a parsed object, so it seems like res.text would be more appropriate for validating the formatting changes.

});

if (options.serverIsStartedInBackground) {
Expand Down
3 changes: 3 additions & 0 deletions packages/server/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
type TypedQueryDocumentNode,
type ValidationContext,
type ValidationRule,
type FormattedExecutionResult,
} from 'graphql';
import {
type KeyValueCache,
Expand Down Expand Up @@ -179,6 +180,7 @@ export interface ApolloServerInternals<TContext extends BaseContext> {
// flip default behavior.
status400ForVariableCoercionErrors?: boolean;
__testing_incrementalExecutionResults?: GraphQLExperimentalIncrementalExecutionResults;
stringifyResult?: (value: FormattedExecutionResult) => string;
}

function defaultLogger(): Logger {
Expand Down Expand Up @@ -332,6 +334,7 @@ export class ApolloServer<in out TContext extends BaseContext = BaseContext> {
config.status400ForVariableCoercionErrors ?? false,
__testing_incrementalExecutionResults:
config.__testing_incrementalExecutionResults,
stringifyResult: config.stringifyResult,
};
}

Expand Down
3 changes: 2 additions & 1 deletion packages/server/src/externalTypes/constructor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type { Logger } from '@apollo/utils.logger';
import type { IExecutableSchemaDefinition } from '@graphql-tools/schema';
import type {
DocumentNode,
FormattedExecutionResult,
GraphQLFieldResolver,
GraphQLFormattedError,
GraphQLSchema,
Expand Down Expand Up @@ -88,7 +89,7 @@ interface ApolloServerOptionsBase<TContext extends BaseContext> {
includeStacktraceInErrorResponses?: boolean;
logger?: Logger;
allowBatchedHttpRequests?: boolean;

stringifyResult?: (value: FormattedExecutionResult) => string;
introspection?: boolean;
plugins?: ApolloServerPlugin<TContext>[];
persistedQueries?: PersistedQueryOptions | false;
Expand Down
8 changes: 5 additions & 3 deletions packages/server/src/runHttpQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,9 +260,11 @@ export async function runHttpQuery<TContext extends BaseContext>({
...graphQLResponse.http,
body: {
kind: 'complete',
string: prettyJSONStringify(
orderExecutionResultFields(graphQLResponse.body.singleResult),
),
string: internals.stringifyResult
? internals.stringifyResult(graphQLResponse.body.singleResult)
: prettyJSONStringify(
orderExecutionResultFields(graphQLResponse.body.singleResult),
),
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I didn't catch this earlier. I'd like to preserve the orderExecutionResultFields call in both cases since that's a spec concern. So maybe we do this instead, let me know what you thing:

  • make the internals.stringifyResult not optional (keep the configuration optional)
  • default the internals property to prettyJSONStringify if it's not configured
  • change the call here to just:
string: internals.stringifyResult(
  orderExecutionResultFields(
    graphQLResponse.body.singleResult,
  ),
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me -- making change now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a peak? :)

},
};
}
Expand Down