Render errors as JSON in http response #5545

newhoggy · 2023-11-20T18:21:05Z

Description

Checklist

Commit sequence broadly makes sense and commits have useful messages
New tests are added if needed and existing tests are updated. These may include:
- golden tests
- property tests
- roundtrip tests
- integration tests
  See Runnings tests for more details
Any changes are noted in the CHANGELOG.md for affected package
The version bounds in .cabal files are updated
CI passes. See note on CI. The following CI checks are required:
- Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
- Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
- Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

.github/workflows/check-stylish-haskell.yml

newhoggy · 2023-12-09T12:12:14Z

cardano-testnet/test/cardano-testnet-test/files/golden/tx.failed.response.yaml.golden

+      kind: ShelleyTxValidationError
+    tag: TxValidationErrorInCardanoMode
+  tag: TxCmdTxSubmitValidationError
+tag: TxSubmitFail


This is the response that will be returned. The golden file is yaml, but the response will be the JSON equivalent.

What's the benefit of yaml vs pretty JSON? My thinking is it's better to be close to the actual output.

carbolymer · 2023-12-11T09:28:33Z

cardano-submit-api/src/Cardano/TxSubmit/Types.hs


 newtype TxSubmitPort = TxSubmitPort Int

 -- | The errors that the raw CBOR transaction parsing\/decoding functions can return.
 --
 newtype RawCborDecodeError = RawCborDecodeError [DecoderError]
-  deriving (Eq, Show)
+  deriving (Eq, Generic, Show)

 instance Error RawCborDecodeError where
  prettyError (RawCborDecodeError decodeErrors) = "RawCborDecodeError decode error: " <> pshow (fmap pshow decodeErrors)


I think this instance can be simplified a bit

Suggested change

prettyError (RawCborDecodeError decodeErrors) = "RawCborDecodeError decode error: " <> pshow (fmap pshow decodeErrors)

prettyError (RawCborDecodeError decodeErrors) = "RawCborDecodeError decode error:" <+> pshow decodeErrors

This instance wasn't added in the PR.

Jimbo4350

Coupe of minor comments

Jimbo4350 · 2023-12-11T14:49:33Z

cardano-submit-api/src/Cardano/TxSubmit/Types.hs

@@ -50,34 +57,40 @@ data TxSubmitWebApiError
  | TxSubmitBadTx !Text
  | TxSubmitFail TxCmdError

-newtype EnvSocketError = CliEnvVarLookup Text deriving (Eq, Show)
+deriving instance Generic TxSubmitWebApiError


I'd rather we take the time to be explicit about ToJSON TxSubmitWebApiError. It gives us better control over deprecation handling or in general customizing the output to our liking.

I can certainly expand this into a manual ToJSON instance. Do you want the ToJSON instance to behave like the generic instance or something different?

I don't have a preference as long as it's explicit

Jimbo4350 · 2023-12-11T14:50:06Z

cardano-submit-api/src/Cardano/TxSubmit/Types.hs


 data TxCmdError
  = TxCmdSocketEnvError EnvSocketError
+  | TxCmdEraConsensusModeMismatch !AnyCardanoEra


This constructor is not used.

Removed. Thanks!

Jimbo4350 · 2023-12-11T14:51:41Z

cardano-testnet/test/cardano-testnet-test/files/golden/tx.failed.response.yaml.golden

+      kind: ShelleyTxValidationError
+    tag: TxValidationErrorInCardanoMode
+  tag: TxCmdTxSubmitValidationError
+tag: TxSubmitFail


What's the benefit of yaml vs pretty JSON? My thinking is it's better to be close to the actual output.

newhoggy · 2023-12-12T07:27:55Z

What's the benefit of yaml vs pretty JSON? My thinking is it's better to be close to the actual output.

I switch to use pretty JSON.

YAML would have some benefit if the JSON string contained newlines, but that might not happen in this case.

Comments addressed

newhoggy force-pushed the newhoggy/structured-errors branch from cbd8143 to bb1e073 Compare November 20, 2023 18:52

newhoggy changed the title ~~Delete unused code~~ Render errors as JSON in http response Nov 20, 2023

newhoggy force-pushed the newhoggy/structured-errors branch from f8d4a96 to 5159934 Compare November 23, 2023 09:47

smelc reviewed Nov 23, 2023

View reviewed changes

.github/workflows/check-stylish-haskell.yml Outdated Show resolved Hide resolved

newhoggy force-pushed the newhoggy/structured-errors branch from 5159934 to 9243464 Compare December 2, 2023 07:21

newhoggy force-pushed the newhoggy/structured-errors branch from 9243464 to f4fb982 Compare December 9, 2023 09:08

newhoggy requested a review from smelc December 9, 2023 09:21

newhoggy force-pushed the newhoggy/structured-errors branch 3 times, most recently from d52ca7a to ebd5dda Compare December 9, 2023 12:03

newhoggy commented Dec 9, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/structured-errors branch from ebd5dda to 9aa39bc Compare December 9, 2023 22:59

carbolymer reviewed Dec 11, 2023

View reviewed changes

carbolymer approved these changes Dec 11, 2023

View reviewed changes

Jimbo4350 previously requested changes Dec 11, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/structured-errors branch from 048fac1 to 8fb75ea Compare December 12, 2023 07:32

newhoggy requested a review from Jimbo4350 December 12, 2023 07:32

Jimbo4350 approved these changes Dec 12, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/structured-errors branch from 31e2073 to 48f4dbb Compare December 12, 2023 13:38

newhoggy added this pull request to the merge queue Dec 13, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 13, 2023

newhoggy added this pull request to the merge queue Dec 20, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 20, 2023

newhoggy force-pushed the newhoggy/structured-errors branch 2 times, most recently from 1ec95fa to 62df968 Compare January 4, 2024 02:07

newhoggy force-pushed the newhoggy/structured-errors branch 2 times, most recently from 867fd60 to b070859 Compare January 12, 2024 12:45

newhoggy added this pull request to the merge queue Jan 12, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 12, 2024

newhoggy added this pull request to the merge queue Jan 13, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 13, 2024

newhoggy added this pull request to the merge queue Jan 13, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 13, 2024

newhoggy added this pull request to the merge queue Jan 14, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 14, 2024

newhoggy added this pull request to the merge queue Jan 14, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 14, 2024

newhoggy added this pull request to the merge queue Jan 14, 2024

newhoggy added 5 commits January 15, 2024 07:57

Delete use of AnyConsensusMode

c0e5207

ToJSON instances for error types

e2aac8f

Use JSON instead of YAML for the golden file

76c58eb

Remove unused constructor

6b44ab7

Explicit ToJSON instance for TxSubmitWebApiError

9d77edc

newhoggy removed this pull request from the merge queue due to a manual request Jan 14, 2024

newhoggy force-pushed the newhoggy/structured-errors branch from b070859 to 9d77edc Compare January 14, 2024 20:58

newhoggy enabled auto-merge January 14, 2024 20:58

newhoggy added this pull request to the merge queue Jan 14, 2024

Merged via the queue into master with commit 54252c5 Jan 15, 2024
19 of 21 checks passed

newhoggy deleted the newhoggy/structured-errors branch January 15, 2024 00:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render errors as JSON in http response #5545

Render errors as JSON in http response #5545

newhoggy commented Nov 20, 2023 •

edited

Loading

newhoggy Dec 9, 2023

Jimbo4350 Dec 11, 2023 •

edited

Loading

carbolymer Dec 11, 2023

newhoggy Dec 12, 2023

Jimbo4350 left a comment

Jimbo4350 Dec 11, 2023

newhoggy Dec 12, 2023

Jimbo4350 Dec 12, 2023

Jimbo4350 Dec 11, 2023

newhoggy Dec 12, 2023

Jimbo4350 Dec 11, 2023 •

edited

Loading

newhoggy commented Dec 12, 2023

	prettyError (RawCborDecodeError decodeErrors) = "RawCborDecodeError decode error: " <> pshow (fmap pshow decodeErrors)
	prettyError (RawCborDecodeError decodeErrors) = "RawCborDecodeError decode error:" <+> pshow decodeErrors

Render errors as JSON in http response #5545

Render errors as JSON in http response #5545

Conversation

newhoggy commented Nov 20, 2023 • edited Loading

Description

Checklist

Note on CI

Choose a reason for hiding this comment

Jimbo4350 Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 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

Jimbo4350 Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

newhoggy commented Dec 12, 2023

newhoggy commented Nov 20, 2023 •

edited

Loading

Jimbo4350 Dec 11, 2023 •

edited

Loading

Jimbo4350 Dec 11, 2023 •

edited

Loading