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

improve shell error handling #330

Merged
merged 2 commits into from
Nov 30, 2023
Merged

improve shell error handling #330

merged 2 commits into from
Nov 30, 2023

Conversation

fauna-chase
Copy link
Contributor

FE-4795
see above jira for more details

  1. shell will not exit on errors
  2. shell will correctly reconnect to core in the presence of an intermittent connection error

j

FE-4795
see above jira for more details

1. shell will not exit on errors
2. shell will correctly reconnect to core in the presence of an
   intermittent connection error
@fauna-chase fauna-chase requested a review from macmv November 28, 2023 17:23
Copy link
Contributor

@macmv macmv left a comment

Choose a reason for hiding this comment

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

I get some compile errors:

src/commands/endpoint/add.ts:123:20 - error TS2339: Property 'close' does not exist on type 'FaunaClient'.

123       await client.close();
                       ~~~~~

and my shell crashes after eval:

Error: client.close is not a function
    at Object.error (/home/macmv/Desktop/programming/fauna/fauna-shell/node_modules/@oclif/core/lib/errors/index.js:27:15)
    at EvalCommand.error (/home/macmv/Desktop/programming/fauna/fauna-shell/node_modules/@oclif/core/lib/command.js:139:23)
    at EvalCommand.error (/home/macmv/Desktop/programming/fauna/fauna-shell/src/lib/fauna-command.js:49:11)
    at EvalCommand.run (/home/macmv/Desktop/programming/fauna/fauna-shell/src/commands/eval.js:115:19)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async EvalCommand._run (/home/macmv/Desktop/programming/fauna/fauna-shell/node_modules/@oclif/core/lib/command.js:117:22)
    at async Config.runCommand (/home/macmv/Desktop/programming/fauna/fauna-shell/node_modules/@oclif/core/lib/config/config.js:329:25)
    at async Object.run (/home/macmv/Desktop/programming/fauna/fauna-shell/node_modules/@oclif/core/lib/main.js:89:16)

Comment on lines 65 to 67
body: {
data: json.data as T,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

this should just be

Suggested change
body: {
data: json.data as T,
},
body: json,

so that the summary field gets copied over. for example, try this: fauna eval "log('hi')"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

o I see, thats why the summary field was shared across error and success types

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah. you can also get log messages back in the summary for runtime errors too:

log("hi"); 1/0

// returns

divide_by_zero: Attempted integer division by zero.

error: Attempted integer division by zero.
at *query*:1:14
  |
1 | log('hi'); 1/0
  |              ^
  |

info at *query*:1: hi

Comment on lines +52 to +55
authorization: `Bearer ${secret ?? this.secret}`,
"x-fauna-source": "Fauna Shell",
...(typecheck !== undefined && { "x-typecheck": typecheck.toString() }),
...(format !== undefined && { "x-format": format }),
Copy link
Contributor

Choose a reason for hiding this comment

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

the x-query-timeout-ms header should be sent here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, looks like we also weren't correctly passing the timeout to the client so resolving that as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh cool, nice

@fauna-chase
Copy link
Contributor Author

yeah my bad, doing my pre commit cleanup I thought I had just removed the old commented out query method but turns out I also removed the non commented out close method.

- use timeout correctly
- add noop client close back
- keep summary for success responses
@fauna-chase fauna-chase requested a review from macmv November 28, 2023 22:29
@fauna-chase fauna-chase merged commit 9ff0cbf into main Nov 30, 2023
@fauna-chase fauna-chase deleted the query-info branch November 30, 2023 21:09
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