[#IP-84] Change strict configuration to true #109

fabriziopapi · 2021-03-30T13:02:45Z

Set the stirct configuration to true.
Fixed the compilation errors caused by the new settings.
The loigc/functions has not been rewritten or refactored: properly type cast has been introduced in order to pass the strict compiler check.

pagopa-github-bot · 2021-03-30T13:04:57Z

	Warnings
⚠️	Please include a Pivotal story at the beginning of the PR title (see below).

Example of PR titles that include pivotal stories:

single story: [#123456] my PR title
multiple stories: [#123456,#123457,#123458] my PR title

New dependencies added: @types/node-fetch.

@types/node-fetch

Author: Unknown

Description: TypeScript definitions for node-fetch

Homepage: http://npmjs.com/package/@types/node-fetch


Created	over 4 years ago
Last Updated	1 day ago
License	MIT
Maintainers	1
Releases	36
Direct Dependencies	`@types/node` and `form-data`

README

Installation

npm install --save @types/node-fetch

Summary

This package contains type definitions for node-fetch (https://github.com/bitinn/node-fetch).

Details

Files were exported from https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/node-fetch.

Additional Details

Last updated: Wed, 07 Apr 2021 23:31:26 GMT
Dependencies: @types/form-data, @types/node
Global values: none

Credits

These definitions were written by Torsten Werner, Niklas Lindgren, Vinay Bedre, Antonio Román, Andrew Leedham, Jason Li, Steve Faulkner, ExE Boss, Alex Savin, Alexis Tyler, and Jakub Kisielewski.

Generated by 🚫 dangerJS

codecov · 2021-03-30T13:05:33Z

Codecov Report

Merging #109 (cb34d59) into master (cad0523) will increase coverage by 0.20%.
The diff coverage is 97.29%.

❗ Current head cb34d59 differs from pull request most recent head ad839f4. Consider uploading reports for the commit ad839f4 to get more accurate results

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   81.82%   82.02%   +0.20%     
==========================================
  Files          27       27              
  Lines         886      907      +21     
  Branches       89       95       +6     
==========================================
+ Hits          725      744      +19     
- Misses        160      162       +2     
  Partials        1        1

Impacted Files	Coverage Δ
GetLimitedProfileByPOST/handler.ts	`83.33% <ø> (ø)`
WebhookNotificationActivity/client.ts	`100.00% <ø> (ø)`
CreateMessage/handler.ts	`63.46% <83.33%> (ø)`
utils/comma-separated-list.ts	`90.00% <90.00%> (ø)`
utils/api.ts	`93.93% <96.00%> (-0.18%)`	⬇️
CreateService/handler.ts	`87.50% <100.00%> (ø)`
EmailNotificationActivity/handler.ts	`71.42% <100.00%> (ø)`
EmailNotificationActivity/utils.ts	`63.15% <100.00%> (ø)`
GetLimitedProfile/handler.ts	`83.87% <100.00%> (ø)`
GetMessage/handler.ts	`87.27% <100.00%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d6b1ce...ad839f4. Read the comment docs.

balanza

Manual casting must be considered the very very last resort. Every time we do that, we are poisoning our type system. It may happen once, but usually it's just problem-shifting - or worse: problem-hiding.

We must use alternatives; not that easy, though.

balanza · 2021-04-07T10:36:54Z

CreateMessage/handler.ts

        name: "api.messages.create",
        properties: {
-          error: isSuccess ? undefined : r.kind,
+          error: isSuccess ? "" : r.kind,


balanza · 2021-04-07T10:43:09Z

CreateService/handler.ts

      apiClient.getUser({
        email: userEmail
-      }),
+      }) as Promise<t.Validation<IResponseType<number, any, never>>>,


We must try not to use manual casting; when we do it, we are injecting unsafe assumptions into the type system, and that makes all other assumptions unsafe.

balanza · 2021-04-07T10:45:35Z

CreatedMessageOrchestrator/utils.ts

  event: MessageProcessingEvent,
  isReplaying: boolean
-): void =>
+): void|null =>


The function can just return void on the dead branch, so you don't have to handle a union type

balanza · 2021-04-07T10:45:46Z

GetService/handler.ts

      apiClient.getService({
        service_id: serviceId
-      }),
+      }) as Promise<t.Validation<IResponseType<number, any, never>>>,


balanza · 2021-04-07T10:45:50Z

GetService/handler.ts

      apiClient.getSubscriptionKeys({
        service_id: serviceId
-      }),
+      }) as Promise<t.Validation<IResponseType<number, any, never>>>,


balanza · 2021-04-07T10:56:04Z

GetUserServices/__tests__/handler.test.ts

    if (result.kind === "IResponseSuccessJson") {
      expect(result.value).toEqual({
-        items: aUserInfo.subscriptions.map(it => it.id)
+        items: (aUserInfo.subscriptions as Subscription[]).map(it => it.id)


ditto: manual casting

balanza · 2021-04-07T10:56:14Z

GetUserServices/handler.ts

      apiClient.getUser({
        email: userEmail
-      }),
+      }) as Promise<t.Validation<IResponseType<number, any, never>>>,


ditto: manual casting

balanza · 2021-04-07T10:56:22Z

GetUserServices/handler.ts

      .map(userInfo =>
        ResponseSuccessJson({
-          items: userInfo.subscriptions.map(it =>
+          items: (userInfo.subscriptions as Subscription[]).map(it =>


ditto: manual casting

balanza · 2021-04-07T10:56:31Z

RegenerateServiceKey/handler.ts

        body: subscriptionKeyTypePayload,
        service_id: serviceId
-      }),
+      }) as Promise<t.Validation<IResponseType<number, any, never>>>,


ditto: manual casting

balanza · 2021-04-07T10:57:03Z

utils/arbitraries.ts

        subject
      }
-    }).getOrElse(undefined)
+    }).fold(l=>undefined, r=>r)


getOrElse is actually a shortcut for this

balanza · 2021-04-08T14:04:56Z

I think we cannot avoid to manual cast things around, at the moment. We can group casts by their source problem into 2 groups:

1. withApiRequestWrapper method fails to narrow the type `T`

This happens because the type T is inferred from the type of apiCallWithParams

Promise<t.Validation<IResponseType<number, T, never>>>

which is not correct, as the payload might be not only T but also other types. A better definition would probably be

Promise<
    | t.Validation<IResponseType<C, T, never>>
    | t.Validation<IResponseType<number, ErrorResponses, never>>
  >

where C is a subset of number which defines our acceptance status (eg: 200).
The problem with the latter definition is that we're no longer able to tell T from T | ErrorResponses - or better: we never were able, but we didn't notice as we weren't on strict mode.
Me and @fabriziopapi spent a morning trying to refactor such functions, but we failed.
The simplest workaround would be to just get rid of it (its only purpose is to avoid too much boilerplate code due to type lifting.

2. `Service` entity is modeled loosey

Service model is defined starting from ServicePayload, which is the DTO we expect from our API's users. To allow partial payloads (that is: you don't need to provide all fields until the service is visible), ServicePayload is defined by the union of VisibleServicePayload and HiddenServicePayload, with the latter having service_metadata nullable. This has been introduced after the discussion in #85.

As a result, api operations such as createService expect either an empty or fully-defined service_metadata field, but as we're reading such field from a given ServicePayload we simply cannot do it, so we arrange an object and we manually cast it as service_metadata like in https://github.com/pagopa/io-functions-services/blob/IP-84--enforce-strict-to-true/createservice/handler.ts#L141.

The issue is the field token_name into service_metadata must never be null, and that conflicts with the above definition; so we have no strong solution other than remodel Service data type.

Given the issue above, I think we should abandon this task and wait until we have them solved.

balanza · 2021-04-08T15:16:16Z

Service entity is modeled loosey

Addendum: one issue is token_name is defined into service_metadata but both can be valued and/or null for independent reasons (that is: token_name can have a value while service_metadata is null). That conflicts with our model which includes the first inside the latter, so that if we have token_name defined and service_metadata undefined, we have to patch a dirty ServiceMetadata object just to include the token. Such operation can be only made with a manual cast.

Are we experiencing production issues? Not at the moment.
The chart below shows the failure count happening on CreateService endpoint exposed by fn-admin (i.e. the one we call from this very application, here for example).
By just reading the model, I would expect some 400 due to the fact that we are eventually sending a ServiceMetadata without a valued scope field; instead, production traffic prove me wrong.

Other said: the case in which we receive an empty service_metadata, even for hidden Services, just never happens.

I don't have enough data to state if it is just contingency and if such event may happen.

…gopa/io-functions-services into IP-84--enforce-strict-to-true

sonarqubecloud · 2021-11-29T09:50:22Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

(^) set strict=true

da61300

fabriziopapi requested review from AleDore, BurnedMarshal, balanza, francescopersico and gunzip as code owners March 30, 2021 13:02

balanza suggested changes Apr 7, 2021

View reviewed changes

balanza added 2 commits April 8, 2021 10:26

Merge branch 'master' into IP-84--enforce-strict-to-true

16471dc

Merge branch 'master' into IP-84--enforce-strict-to-true

da1d990

fabriziopapi added 5 commits April 8, 2021 18:48

(^) fixed withApiRequestWrapper strict=true compatibility

14e1dec

Merge branch 'IP-84--enforce-strict-to-true' of https://github.com/pa…

2704fcf

…gopa/io-functions-services into IP-84--enforce-strict-to-true

(+) add docblock to withApiRequestWrapper function

cb2283b

(+) added nw api wrapper function to managen empty-response call

cb34d59

(^) fixed trackMessageProcessing return type

ad839f4

balanza marked this pull request as draft April 27, 2021 12:32

balanza added the invalid This doesn't seem right label Apr 27, 2021

[#IP-84] Change strict configuration to true #109

Are you sure you want to change the base?

[#IP-84] Change strict configuration to true #109

Uh oh!

Conversation

fabriziopapi commented Mar 30, 2021

Uh oh!

pagopa-github-bot commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Installation

Summary

Details

Additional Details

Credits

Uh oh!

codecov bot commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

balanza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balanza commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. withApiRequestWrapper method fails to narrow the type T

2. Service entity is modeled loosey

Uh oh!

balanza commented Apr 8, 2021

Uh oh!

sonarqubecloud bot commented Nov 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pagopa-github-bot commented Mar 30, 2021 •

edited

Loading

codecov bot commented Mar 30, 2021 •

edited

Loading

balanza commented Apr 8, 2021 •

edited

Loading

1. withApiRequestWrapper method fails to narrow the type `T`

2. `Service` entity is modeled loosey