Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[CBR-306] Akegalj/co 319/swagger account index #3086

Conversation

akegalj
Copy link
Contributor

@akegalj akegalj commented Jun 13, 2018

Description

The example accountId from the API doc isn't much relevant and reflects an invalid value

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CO-319

Type of change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • πŸ›  New feature (non-breaking change which adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • πŸ”¨ New or improved tests for existing code
  • β›‘ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

  1. Run a cardano-node
  2. Visit https://localhost:8091/docs/v1/index/ - you will see changes showed bellow
  3. Roundtrip tests pass (the screenshot bellow)

Screenshots (if available)

accountIndex in request body shows correct value range
look

accountIndex on description on the right picks correct value (not bellow the minimum)
look

accountId (same thing as accountIndex) in request params shows correct value range
look

roundtrip tests
look

@KtorZ I wonder why did we ended up with accountId and accountIndex json keys for the same thing. Is it still too late to unify it? (it would be a breaking change for exchanges)

mkAccountIndex :: Word32 -> Either Text AccountIndex
mkAccountIndex index | index >= getAccIndex minBound = Right $ AccountIndex index
| otherwise = Left $ sformat
("mkAccountIndex: Account index should be in range ["%int%".."%int%"]")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we think it make sense to add additional constructor to WalletError which would capture this failure?

There are other places in this module where I can do this refactoring (possibly as part of https://iohk.myjetbrains.com/youtrack/issue/CBR-26 )

Copy link
Contributor

Choose a reason for hiding this comment

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

Oui :)
IMO, we should always favor explicit and typed error when we can. Especially when the error is a well-identified scenario that is reachable.

@@ -186,17 +186,13 @@ instance (HasSwagger subApi) => HasSwagger (WalletRequestParams :> subApi) where

instance ToParamSchema WalletId

instance ToSchema Core.Address where
declareNamedSchema = pure . paramSchemaToNamedSchema defaultSchemaOptions

instance ToParamSchema Core.Address where
Copy link
Contributor

Choose a reason for hiding this comment

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

😢 Why exactly? Is this not used?

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 catch, looks like a mistake

mkAccountIndex :: Word32 -> Either Text AccountIndex
mkAccountIndex index | index >= getAccIndex minBound = Right $ AccountIndex index
| otherwise = Left $ sformat
("mkAccountIndex: Account index should be in range ["%int%".."%int%"]")
Copy link
Contributor

Choose a reason for hiding this comment

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

Oui :)
IMO, we should always favor explicit and typed error when we can. Especially when the error is a well-identified scenario that is reachable.


instance FromJSON AccountIndex where
parseJSON =
either (fail . toString) pure . mkAccountIndex <=< parseJSON
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that if we define an explicit WalletError for mkAccountIndex, it'd be better to use sformat instead of toString to leverage a Buildable instance and have a proper diagnostic!

deriveSafeBuildable ''AccountIndex
-- Nothing secret to redact for a AccountIndex.
instance BuildableSafeGen AccountIndex where
buildSafeGen _ = bprint build
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... isn't that a bit too recursive ^^" ?

Copy link
Contributor Author

@akegalj akegalj Jun 18, 2018

Choose a reason for hiding this comment

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

uh, you might be right!

I saw a similar definition for SyncProgress in the same module

buildSafeGen _ sp = bprint build sp
. Let me have a look at how BuildableSafeGen is defined...

EDIT: Good catch. In fact, SyncProgress will also recurse.

httpApiDataRoundtripProp @(V1 Core.Address) Proxy
httpApiDataRoundtripProp @PerPage Proxy
httpApiDataRoundtripProp @Page Proxy
httpApiDataRoundtripProp @Core.Coin Proxy
Copy link
Contributor

Choose a reason for hiding this comment

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

πŸ‘Œ

@KtorZ
Copy link
Contributor

KtorZ commented Jun 18, 2018

@KtorZ I wonder why did we ended up with accountId and accountIndex json keys for the same thing. Is it still too late to unify it? (it would be a breaking change for exchanges)

Erf :/ ... Not sure introducing a breaking change will be well-advised here. Especially for a JSON-key which can be hard to chase in exchange integrations. Yet, we can perhaps have a backward compatible fix making a slightly hacky parser accepting both accountId and accountIndex keys with a clear comment about it.

@@ -688,7 +707,14 @@ instance ToSchema SyncProgress where
deriveSafeBuildable ''SyncProgress
-- Nothing secret to redact for a SyncProgress.
instance BuildableSafeGen SyncProgress where
buildSafeGen _ sp = bprint build sp
buildSafeGen sl SyncProgress {..} = bprint ("{"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

es Util WalletSpecs Data.Text.Buildable Test.QuickCheck> sp :: SyncProgress  <- generate arbitrary                                                            
es Util WalletSpecs Data.Text.Buildable Test.QuickCheck> sp                                                                                                   
SyncProgress {spEstimatedCompletionTime = EstimatedCompletionTime (MeasuredIn 2894754), spThroughput = SyncThroughput (MeasuredIn (SyncThroughput (BlockCount {getBlockCount = 12610705}))), spPercentage = SyncPercentage (MeasuredIn 42)}
es Util WalletSpecs Data.Text.Buildable Test.QuickCheck> build sp                                                                                             
"{ estimatedCompletionTime={ quantity=2894754 unit=milliseconds } throughput={ quantity=12610705 unit=blocksPerSecond } percentage=42% }"

it was recursive before

deriveSafeBuildable ''AccountIndex
-- Nothing secret to redact for a AccountIndex.
instance BuildableSafeGen AccountIndex where
buildSafeGen _ = bprint build . getAccIndex
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed recursive buildable. Thanks for catching that!

es Util WalletSpecs Data.Text.Buildable Test.QuickCheck> let ac = fromRight (error "bla") (mkAccountIndex 2147483649)                                         
es Util WalletSpecs Data.Text.Buildable Test.QuickCheck> ac                                                                                                   
AccountIndex {getAccIndex = 2147483649}
es Util WalletSpecs Data.Text.Buildable Test.QuickCheck> build ac                                                                                             
"2147483649"

@akegalj akegalj added the wip label Jun 18, 2018
@parsonsmatt
Copy link
Contributor

@KtorZ I've implemented the ADT-error-type; care to re-review?

@KtorZ
Copy link
Contributor

KtorZ commented Jun 22, 2018

LGTM πŸ‘

@KtorZ
Copy link
Contributor

KtorZ commented Jun 22, 2018

Some second thoughts, @parsonsmatt What's the rational behind a new AccountIndexError data-type vs a new constructor in the WalletError type?

@@ -195,7 +195,7 @@ instance Migrate V0.AccountId (V1.WalletId, V1.AccountIndex) where
(,)
<$> eitherMigrate (V0.aiWId accId)
<*> first
Errors.MigrationFailed
(Errors.MigrationFailed . sformat build)
Copy link
Contributor Author

@akegalj akegalj Jun 22, 2018

Choose a reason for hiding this comment

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

do you think it makes sense to introduce new constructor in WalletError to capture exact underying issue (instead of wrapping it under MigrationFailed). I wonder does it makes sense to have this pattern in general.

This would make it possible to have more detailed error handling if it is needed. Now we mask underlying errors under higher level error (which is MigrationFailed in this case).

I wonder do we have some opinion/rules about this?

@akegalj
Copy link
Contributor Author

akegalj commented Jun 22, 2018

@KtorZ

Some second thoughts, @parsonsmatt What's the rational behind a new AccountIndexError data-type vs a new constructor in the WalletError type?

problem is with cyclic imports. V1.Errors module is importing V1.Types so we are unable to import and use WalletError inside V1.Types. We would have to factor out mkAccountIndex constructor out of V1.Types. I wonder would this be something preferable? Also I am curious about #3086 (comment) approach ?

--
-- Error from parsing / validating JSON inputs
--

Copy link
Contributor Author

@akegalj akegalj Jun 25, 2018

Choose a reason for hiding this comment

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

@KtorZ I see you have made MigrationFailed an instance of Expcetion , but JSONValidationError doesn't have the same instance. Is there a reason for the separation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm. No. That's an oversight πŸ‘

@KtorZ KtorZ force-pushed the akegalj/CO-319/swagger-account-index branch from 21598f4 to 6161cb7 Compare June 25, 2018 16:01
@akegalj akegalj requested a review from vincenthz as a code owner June 25, 2018 16:01
@parsonsmatt
Copy link
Contributor

@KtorZ Clarity. If I say:

mkAccountIndex :: Int -> Either AnyApplicationError AccountIndex

then you lose all type-safety around what exceptions you should be throwing, and you lose type-safety in handling the appropriate exceptions.

@KtorZ KtorZ changed the base branch from Squad1/CO-309/misleading-example-for-account-id to Squad1/CBR-26/improve-error-handling June 26, 2018 08:32
@KtorZ KtorZ force-pushed the akegalj/CO-319/swagger-account-index branch 2 times, most recently from d93784b to 2924c46 Compare June 27, 2018 12:58
akegalj and others added 8 commits June 27, 2018 16:48
This makes it now possible to import V1/Errors from the V1/Types module and leverage errors from this module.
One thing is still unclear to me: Why Errors isn't defined in V1/Types already?
There's a circular dependency between V1/Response and V1/Types if we go this way, as well as between
V1/Migration and V1/Types.
Nevertheless, it would make sense to have three data-types here:

- WalletError (defined in V1/Types)
- MigrationError (defined in V1/Types)
- JSONParsingError (defined in Response)

This way, we could remove the conflicting constructor from WalletError and remove the need for an
extra module here. It will also makes thing clearer
To realize this, we had to extract JSONValidationFailed and MigrationFailed constructor from WalletError.
They're now defined as constructor in different data-types (resp. JSONValidationError and MigrationError).
This is rather ugly and could probably be achieved nicely with a better understanding of the
Generics.SOP library. As far as I could tell, there's no easy way to retrieve 'Tag' for single
constructor

(cf: 'For a datatype with a single constructor we do not need to tag values with their constructor; but for a datatype with multiple constructors we do.  ')
@KtorZ KtorZ force-pushed the akegalj/CO-319/swagger-account-index branch from 2924c46 to 808f00b Compare June 27, 2018 15:08
Copy link
Contributor Author

@akegalj akegalj left a comment

Choose a reason for hiding this comment

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

@KtorZ @parsonsmatt thanks for jumping in. LGTM

@akegalj akegalj merged commit 8899c1f into Squad1/CBR-26/improve-error-handling Jun 28, 2018
@akegalj akegalj removed the wip label Jun 28, 2018
@KtorZ KtorZ deleted the akegalj/CO-319/swagger-account-index branch June 28, 2018 12:54
KtorZ pushed a commit that referenced this pull request Jul 2, 2018
* [CO-319] Fix account index swagger example

* [CO-319] Add roundtrip tests

* [CO-319] Fix recursive buildable instances

* [CO-319] Use strongly typed error

* [CO-319] Remove duplication in 'renderAccountIndexError'

* [CO-319] Distangle V1/Errors

This makes it now possible to import V1/Errors from the V1/Types module and leverage errors from this module.
One thing is still unclear to me: Why Errors isn't defined in V1/Types already?
There's a circular dependency between V1/Response and V1/Types if we go this way, as well as between
V1/Migration and V1/Types.
Nevertheless, it would make sense to have three data-types here:

- WalletError (defined in V1/Types)
- MigrationError (defined in V1/Types)
- JSONParsingError (defined in Response)

This way, we could remove the conflicting constructor from WalletError and remove the need for an
extra module here. It will also makes thing clearer

* [CO-319] Make V1/Error part of V1/Types

To realize this, we had to extract JSONValidationFailed and MigrationFailed constructor from WalletError.
They're now defined as constructor in different data-types (resp. JSONValidationError and MigrationError).

* [CO-319] Solve rebase conflicts

* [CO-319] Correctly format (jsend) newtype errors

This is rather ugly and could probably be achieved nicely with a better understanding of the
Generics.SOP library. As far as I could tell, there's no easy way to retrieve 'Tag' for single
constructor

(cf: 'For a datatype with a single constructor we do not need to tag values with their constructor; but for a datatype with multiple constructors we do.  ')
@akegalj akegalj changed the title Akegalj/co 319/swagger account index [CBR-319] Akegalj/co 319/swagger account index Jul 2, 2018
@akegalj akegalj changed the title [CBR-319] Akegalj/co 319/swagger account index [CBR-306] Akegalj/co 319/swagger account index Jul 2, 2018
KtorZ pushed a commit that referenced this pull request Jul 9, 2018
* [CO-319] Fix account index swagger example

* [CO-319] Add roundtrip tests

* [CO-319] Fix recursive buildable instances

* [CO-319] Use strongly typed error

* [CO-319] Remove duplication in 'renderAccountIndexError'

* [CO-319] Distangle V1/Errors

This makes it now possible to import V1/Errors from the V1/Types module and leverage errors from this module.
One thing is still unclear to me: Why Errors isn't defined in V1/Types already?
There's a circular dependency between V1/Response and V1/Types if we go this way, as well as between
V1/Migration and V1/Types.
Nevertheless, it would make sense to have three data-types here:

- WalletError (defined in V1/Types)
- MigrationError (defined in V1/Types)
- JSONParsingError (defined in Response)

This way, we could remove the conflicting constructor from WalletError and remove the need for an
extra module here. It will also makes thing clearer

* [CO-319] Make V1/Error part of V1/Types

To realize this, we had to extract JSONValidationFailed and MigrationFailed constructor from WalletError.
They're now defined as constructor in different data-types (resp. JSONValidationError and MigrationError).

* [CO-319] Solve rebase conflicts

* [CO-319] Correctly format (jsend) newtype errors

This is rather ugly and could probably be achieved nicely with a better understanding of the
Generics.SOP library. As far as I could tell, there's no easy way to retrieve 'Tag' for single
constructor

(cf: 'For a datatype with a single constructor we do not need to tag values with their constructor; but for a datatype with multiple constructors we do.  ')
KtorZ pushed a commit that referenced this pull request Jul 25, 2018
* [CO-319] Fix account index swagger example

* [CO-319] Add roundtrip tests

* [CO-319] Fix recursive buildable instances

* [CO-319] Use strongly typed error

* [CO-319] Remove duplication in 'renderAccountIndexError'

* [CO-319] Distangle V1/Errors

This makes it now possible to import V1/Errors from the V1/Types module and leverage errors from this module.
One thing is still unclear to me: Why Errors isn't defined in V1/Types already?
There's a circular dependency between V1/Response and V1/Types if we go this way, as well as between
V1/Migration and V1/Types.
Nevertheless, it would make sense to have three data-types here:

- WalletError (defined in V1/Types)
- MigrationError (defined in V1/Types)
- JSONParsingError (defined in Response)

This way, we could remove the conflicting constructor from WalletError and remove the need for an
extra module here. It will also makes thing clearer

* [CO-319] Make V1/Error part of V1/Types

To realize this, we had to extract JSONValidationFailed and MigrationFailed constructor from WalletError.
They're now defined as constructor in different data-types (resp. JSONValidationError and MigrationError).

* [CO-319] Solve rebase conflicts

* [CO-319] Correctly format (jsend) newtype errors

This is rather ugly and could probably be achieved nicely with a better understanding of the
Generics.SOP library. As far as I could tell, there's no easy way to retrieve 'Tag' for single
constructor

(cf: 'For a datatype with a single constructor we do not need to tag values with their constructor; but for a datatype with multiple constructors we do.  ')
KtorZ pushed a commit that referenced this pull request Aug 17, 2018
* [CO-319] Fix account index swagger example

* [CO-319] Add roundtrip tests

* [CO-319] Fix recursive buildable instances

* [CO-319] Use strongly typed error

* [CO-319] Remove duplication in 'renderAccountIndexError'

* [CO-319] Distangle V1/Errors

This makes it now possible to import V1/Errors from the V1/Types module and leverage errors from this module.
One thing is still unclear to me: Why Errors isn't defined in V1/Types already?
There's a circular dependency between V1/Response and V1/Types if we go this way, as well as between
V1/Migration and V1/Types.
Nevertheless, it would make sense to have three data-types here:

- WalletError (defined in V1/Types)
- MigrationError (defined in V1/Types)
- JSONParsingError (defined in Response)

This way, we could remove the conflicting constructor from WalletError and remove the need for an
extra module here. It will also makes thing clearer

* [CO-319] Make V1/Error part of V1/Types

To realize this, we had to extract JSONValidationFailed and MigrationFailed constructor from WalletError.
They're now defined as constructor in different data-types (resp. JSONValidationError and MigrationError).

* [CO-319] Solve rebase conflicts

* [CO-319] Correctly format (jsend) newtype errors

This is rather ugly and could probably be achieved nicely with a better understanding of the
Generics.SOP library. As far as I could tell, there's no easy way to retrieve 'Tag' for single
constructor

(cf: 'For a datatype with a single constructor we do not need to tag values with their constructor; but for a datatype with multiple constructors we do.  ')
KtorZ pushed a commit that referenced this pull request Aug 17, 2018
* [CO-319] Fix account index swagger example

* [CO-319] Add roundtrip tests

* [CO-319] Fix recursive buildable instances

* [CO-319] Use strongly typed error

* [CO-319] Remove duplication in 'renderAccountIndexError'

* [CO-319] Distangle V1/Errors

This makes it now possible to import V1/Errors from the V1/Types module and leverage errors from this module.
One thing is still unclear to me: Why Errors isn't defined in V1/Types already?
There's a circular dependency between V1/Response and V1/Types if we go this way, as well as between
V1/Migration and V1/Types.
Nevertheless, it would make sense to have three data-types here:

- WalletError (defined in V1/Types)
- MigrationError (defined in V1/Types)
- JSONParsingError (defined in Response)

This way, we could remove the conflicting constructor from WalletError and remove the need for an
extra module here. It will also makes thing clearer

* [CO-319] Make V1/Error part of V1/Types

To realize this, we had to extract JSONValidationFailed and MigrationFailed constructor from WalletError.
They're now defined as constructor in different data-types (resp. JSONValidationError and MigrationError).

* [CO-319] Solve rebase conflicts

* [CO-319] Correctly format (jsend) newtype errors

This is rather ugly and could probably be achieved nicely with a better understanding of the
Generics.SOP library. As far as I could tell, there's no easy way to retrieve 'Tag' for single
constructor

(cf: 'For a datatype with a single constructor we do not need to tag values with their constructor; but for a datatype with multiple constructors we do.  ')
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants