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.

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

fix: Update graphql-js error handling #3132

Merged
merged 3 commits into from
Jul 20, 2023
Merged

fix: Update graphql-js error handling #3132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e37111
fix: Update graphql-js error handling
BobaFetters Jul 20, 2023
28e0d78
Minor test fix
BobaFetters Jul 20, 2023
6354227
Removing [Error] label
BobaFetters Jul 20, 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
10 changes: 8 additions & 2 deletions Sources/ApolloCodegenLib/ApolloCodegen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,14 @@ public class ApolloCodegen {
)

guard graphqlErrors.isEmpty else {
let errorlines = graphqlErrors.flatMap({ $0.logLines })
CodegenLogger.log(String(describing: errorlines), logLevel: .error)
let errorlines = graphqlErrors.flatMap({
Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking here is by making the sourceLocations optional and therefore the logLines optional, we can determine how to display a given error, so errors with sourceLocations get displayed as they always have, but errors without sourceLocations no longer cause a precondition failure and instead just have their name and message displayed to ensure the user always gets usable error information

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 an awesome solution!

if let logLines = $0.logLines {
return logLines
} else {
return ["\($0.name ?? "unknown"): \($0.message ?? "")"]
}
})
CodegenLogger.log(errorlines.joined(separator: "\n"), logLevel: .error)
throw Error.graphQLSourceValidationFailure(atLines: errorlines)
}

Expand Down
10 changes: 6 additions & 4 deletions Sources/ApolloCodegenLib/Frontend/GraphQLError.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@ public class GraphQLError: JavaScriptError {
private lazy var source: GraphQLSource = self["source"]

/// The source locations associated with this error.
private(set) lazy var sourceLocations: [GraphQLSourceLocation] = {
let locations: [JavaScriptObject] = self["locations"]
private(set) lazy var sourceLocations: [GraphQLSourceLocation]? = {
guard let locations: [JavaScriptObject] = self["locations"] else {
return nil
}

if let nodes: [ASTNode] = self["nodes"] {
// We have AST nodes, so this is a validation error.
Expand All @@ -35,8 +37,8 @@ public class GraphQLError: JavaScriptError {

/// Log lines for this error in a format that allows Xcode to show errors inline at the correct location.
/// See https://shazronatadobe.wordpress.com/2010/12/04/xcode-shell-build-phase-reporting-of-errors/
var logLines: [String] {
return sourceLocations.map {
var logLines: [String]? {
return sourceLocations?.map {
return [$0.filePath, String($0.lineNumber), "error", message ?? "?"].joined(separator: ":")
}
}
Expand Down
6 changes: 3 additions & 3 deletions Tests/ApolloCodegenTests/Frontend/SchemaLoadingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ class SchemaLoadingTests: XCTestCase {
let error = try XCTDowncast(error as AnyObject, to: GraphQLError.self)
XCTAssert(try XCTUnwrap(error.message).starts(with: "Syntax Error"))

XCTAssertEqual(error.sourceLocations.count, 1)
XCTAssertEqual(error.sourceLocations[0].filePath, "schema.graphqls")
XCTAssertEqual(error.sourceLocations[0].lineNumber, 3)
XCTAssertEqual(error.sourceLocations?.count, 1)
XCTAssertEqual(error.sourceLocations?[0].filePath, "schema.graphqls")
XCTAssertEqual(error.sourceLocations?[0].lineNumber, 3)
}
}
}
Expand Down