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

style(examples): 2021-09-20 linter warnings batch 17 / 26; part 2 #2425

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

adrianbatuto
Copy link
Contributor

@adrianbatuto adrianbatuto commented May 15, 2023

  1. Added a user-defined type guard to check for BambooHarvest types
    on the front-end.
  2. Additional runtime type checks during data type conversion so that
    if we get invalid input it speaks up against it specifically instead of
    using any types and generic crashes.

Fixes #2092

Co-authored-by: Peter Somogyvari peter.somogyvari@accenture.com

Signed-off-by: adrianbatuto adrian.batuto@accenture.com
Signed-off-by: Peter Somogyvari peter.somogyvari@accenture.com

@adrianbatuto adrianbatuto changed the title style: 2021-09-20 linter warnings batch 17 / 26; part 2 style: 2021-09-20 linter warnings batch 17 / 26; part 2 May 15, 2023
@adrianbatuto adrianbatuto marked this pull request as ready for review May 16, 2023 07:48
@jagpreetsinghsasan
Copy link
Contributor

@adrianbatuto thankyou for the PR !
I would suggest you to squash merge the commits into 1, alongside proper PR description and commit title & description. For reference: #2272 (you can check the PR description and the commit title & description in this PR).

@adrianbatuto
Copy link
Contributor Author

@adrianbatuto thankyou for the PR ! I would suggest you to squash merge the commits into 1, alongside proper PR description and commit title & description. For reference: #2272 (you can check the PR description and the commit title & description in this PR).

Thanks @jagpreetsinghsasan. I have made some updates to the pr.

@petermetz
Copy link
Member

Suggested edit:

diff --git a/examples/cactus-example-supply-chain-business-logic-plugin/src/main/typescript/model/converter/bamboo-harvest-converter.ts b/examples/cactus-example-supply-chain-business-logic-plugin/src/main/typescript/model/converter/bamboo-harvest-converter.ts
index 7aa858086..0b2981a63 100644
--- a/examples/cactus-example-supply-chain-business-logic-plugin/src/main/typescript/model/converter/bamboo-harvest-converter.ts
+++ b/examples/cactus-example-supply-chain-business-logic-plugin/src/main/typescript/model/converter/bamboo-harvest-converter.ts
@@ -1,4 +1,5 @@
 import { BambooHarvest } from "../../generated/openapi/typescript-axios";
+import { RuntimeError } from 'run-time-error';
 
 /**
  * Responsible for converting model entities such as the `BambooHarvest` to and
@@ -22,9 +23,21 @@ export class BambooHarvestConverter {
    * `BambooHarvest` model entity.
    */
   public static ofSolidityStruct(arr: unknown[]): BambooHarvest {
+    const id = arr[BambooHarvestConverter.SOLIDITY_FIELD_ID];
+    if (typeof id !== "string") {
+      const errMsg = `Expected the value of arr[${BambooHarvestConverter.SOLIDITY_FIELD_ID}] to be a string`;
+      throw new RuntimeError(errMsg);
+    }
+
+    const location = arr[BambooHarvestConverter.SOLIDITY_FIELD_LOCATION];
+    if (typeof location !== "string") {
+      const errMsg = `Expected the value of arr[${BambooHarvestConverter.SOLIDITY_FIELD_LOCATION}] to be a string`;
+      throw new RuntimeError(errMsg);
+    }
+
     return {
-      id: arr[BambooHarvestConverter.SOLIDITY_FIELD_ID] as string,
-      location: arr[BambooHarvestConverter.SOLIDITY_FIELD_LOCATION] as string,
+      id,
+      location,
       startedAt: arr[
         BambooHarvestConverter.SOLIDITY_FIELD_STARTED_AT
       ] as string,

Copy link
Member

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

Please see my suggestion and make sure to apply the same logic for the rest of the fields e.g. these below:

startedAt: arr[
        BambooHarvestConverter.SOLIDITY_FIELD_STARTED_AT
      ] as string,
      endedAt: arr[BambooHarvestConverter.SOLIDITY_FIELD_ENDED_AT] as string,
      harvester: arr[BambooHarvestConverter.SOLIDITY_FIELD_HARVESTER] as string,

Copy link
Member

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@adrianbatuto I added a couple more comments here, please see above.

@adrianbatuto adrianbatuto force-pushed the adrianbatuto/issue2092 branch 2 times, most recently from 35e19c6 to 756c56d Compare June 15, 2023 06:41
Copy link
Member

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@adrianbatuto See my comments above please

1. Added a user-defined type guard to check for `BambooHarvest` types
on the front-end.
2. Additional runtime type checks during data type conversion so that
if we get invalid input it speaks up against it specifically instead of
using `any` types and generic crashes.

Fixes hyperledger#2092

Co-authored-by: Peter Somogyvari <peter.somogyvari@accenture.com>

Signed-off-by: adrianbatuto <adrian.batuto@accenture.com>
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz petermetz changed the title style: 2021-09-20 linter warnings batch 17 / 26; part 2 style(examples): 2021-09-20 linter warnings batch 17 / 26; part 2 Jun 27, 2023
Copy link
Member

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@adrianbatuto nvm, I pushed some fixes just now so that we can get this over the line ASAP. Apart from my comments above the other thing that I changed is that I moved the user-defined type guard to it's own code-file and exported it so that it can be re-used elsewhere.

@petermetz petermetz enabled auto-merge (rebase) June 27, 2023 20:13
@petermetz petermetz merged commit 398d8b2 into hyperledger:main Jun 27, 2023
109 of 117 checks passed
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.

style: 2021-09-20 linter warnings batch 17 / 26; part 2
4 participants