Skip to content

Federation: multi-line description bug #3828

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Federation: multi-line description bug #3828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d54e35
Fix descriptions with double quotes
trevor-scheer Feb 25, 2020
5eb8206
Fix snapshot indentation
trevor-scheer Feb 25, 2020
b213bdc
Update changelog
trevor-scheer Feb 25, 2020
d3631b2
Merge branch 'master' into trevor/federation-description-bug
trevor-scheer Feb 25, 2020
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
2 changes: 1 addition & 1 deletion packages/apollo-federation/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

> The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. When a release is being prepared, a new header will be (manually) created below and the the appropriate changes within that release will be moved into the new section.

- _Nothing yet! Stay tuned._
- Fix multi-line descriptions which use double quotes [#3828](https://github.com/apollographql/apollo-server/pull/3828)

## v0.12.1

Expand Down
141 changes: 85 additions & 56 deletions packages/apollo-federation/src/service/__tests__/buildFederatedSchema.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ describe('buildFederatedSchema', () => {
`);

expect(schema.getType('Product')).toMatchInlineSnapshot(`
type Product {
upc: String!
name: String
price: Int
}
`);
type Product {
upc: String!
name: String
price: Int
}
`);

expect(schema.getType('_Entity')).toMatchInlineSnapshot(
`union _Entity = Product`,
Expand All @@ -44,13 +44,13 @@ type Product {
`);

expect(schema.getType('Product')).toMatchInlineSnapshot(`
type Product {
upc: String!
sku: String!
name: String
price: Int
}
`);
type Product {
upc: String!
sku: String!
name: String
price: Int
}
`);

expect(schema.getType('_Entity')).toMatchInlineSnapshot(
`union _Entity = Product`,
Expand All @@ -66,11 +66,11 @@ type Product {
`);

expect(schema.getType('Money')).toMatchInlineSnapshot(`
type Money {
amount: Int!
currencyCode: String!
}
`);
type Money {
amount: Int!
currencyCode: String!
}
`);
});

it('should preserve description text in generated SDL', async () => {
Expand Down Expand Up @@ -128,6 +128,35 @@ type User @key(fields: "id") {
`);
});

it('should preserve multiline descriptions correctly (and with quotes)', async () => {
const query = `query GetServiceDetails {
_service {
sdl
}
}`;
const schema = buildFederatedSchema(gql`
type User {
"""
The unique ID of the "user"
"""
id: ID!
}
`);

const { data, errors } = await graphql(schema, query);
expect(errors).toBeUndefined();

expect(data._service.sdl).toMatchInlineSnapshot(`
"type User {
\\"\\"\\"
The unique ID of the \\"user\\"
\\"\\"\\"
id: ID!
}
"
`);
});

describe(`should add an _entities query root field to the schema`, () => {
it(`when a query root type with the default name has been defined`, () => {
const schema = buildFederatedSchema(gql`
Expand All @@ -140,12 +169,12 @@ type User @key(fields: "id") {
`);

expect(schema.getQueryType()).toMatchInlineSnapshot(`
type Query {
_entities(representations: [_Any!]!): [_Entity]!
_service: _Service!
rootField: String
}
`);
type Query {
_entities(representations: [_Any!]!): [_Entity]!
_service: _Service!
rootField: String
}
`);
});

it(`when a query root type with a non-default name has been defined`, () => {
Expand All @@ -163,23 +192,23 @@ type Query {
`);

expect(schema.getQueryType()).toMatchInlineSnapshot(`
type QueryRoot {
_entities(representations: [_Any!]!): [_Entity]!
_service: _Service!
rootField: String
}
`);
type QueryRoot {
_entities(representations: [_Any!]!): [_Entity]!
_service: _Service!
rootField: String
}
`);
});
});
describe(`should not add an _entities query root field to the schema`, () => {
it(`when no query root type has been defined`, () => {
const schema = buildFederatedSchema(EMPTY_DOCUMENT);

expect(schema.getQueryType()).toMatchInlineSnapshot(`
type Query {
_service: _Service!
}
`);
type Query {
_service: _Service!
}
`);
});
it(`when no types with keys are found`, () => {
const schema = buildFederatedSchema(gql`
Expand All @@ -189,11 +218,11 @@ type Query {
`);

expect(schema.getQueryType()).toMatchInlineSnapshot(`
type Query {
_service: _Service!
rootField: String
}
`);
type Query {
_service: _Service!
rootField: String
}
`);
});
it(`when only an interface with keys are found`, () => {
const schema = buildFederatedSchema(gql`
Expand All @@ -206,11 +235,11 @@ type Query {
`);

expect(schema.getQueryType()).toMatchInlineSnapshot(`
type Query {
_service: _Service!
rootField: String
}
`);
type Query {
_service: _Service!
rootField: String
}
`);
});
});
describe('_entities root field', () => {
Expand Down Expand Up @@ -601,25 +630,25 @@ describe('legacy interface', () => {
`,
});
expect(schema.getType('Product')).toMatchInlineSnapshot(`
type Product {
upc: String!
name: String
price: Int
}
`);
type Product {
upc: String!
name: String
price: Int
}
`);
expect(schema.getType('_Entity')).toMatchInlineSnapshot(
`union _Entity = Product`,
);
});
it('allows legacy schema module interface as a simple array of documents', async () => {
const schema = buildFederatedSchema({ typeDefs });
expect(schema.getType('Product')).toMatchInlineSnapshot(`
type Product {
upc: String!
name: String
price: Int
}
`);
type Product {
upc: String!
name: String
price: Int
}
`);
expect(schema.getType('_Entity')).toMatchInlineSnapshot(
`union _Entity = Product`,
);
Expand Down
7 changes: 6 additions & 1 deletion packages/apollo-federation/src/service/printFederatedSchema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,12 @@ function printDescription(
}

const lines = descriptionLines(def.description, 120 - indentation.length);
if (lines.length === 1) {

// If a description uses the double quote character, it absolutely must use
// a multi-line comment.
const requireMultiline = lines.some(line => line.includes('"'));

if (lines.length === 1 && !requireMultiline) {
Comment on lines +351 to +355
Copy link
Member

@abernix abernix Feb 25, 2020

Choose a reason for hiding this comment

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

Escaped quotes (i.e. \") are valid in non-block'd def.description.values in a schema's AST, so the comment here is not quite correct. Further, I believe by using this implementation we'd be applying/requiring this transformation unnecessarily.

Perhaps we might consider downstreaming some of the changes in graphql's printSchema which seems to have a more current version of the printDescription method which I believe we cargo cult-ed at one point.

Generally, except for where we leverage specific overrides, I'd have a preference for a more exact copy of that implementation.

return indentation + `"${lines[0]}"\n`;
} else {
return (
Expand Down