Check if properRoute is found before acting on it #73

kdhira · 2022-02-21T22:44:01Z

In the cases where the request route is not matched to any Koa route,
bail out before trying to format the path string.

Issue: #59

In the cases where the request route is not matched to any Koa route, bail out before trying to format the path string. Issue: PayU#59

kdhira · 2022-02-21T22:50:22Z

src/koa-middleware.js

+        // If proper route is not found, send an undefined route
+        // The caller is responsible for setting a default "N/A" route in this case
+        if (!properRoute) {
+            return undefined;


In the _handleResponse method, this will set the route label to 'N/A' though the || operator. Let me know if there is a cleaner solution

kdhira · 2022-02-26T01:46:15Z

@kobik can you take a look at this?

kobik · 2022-02-26T07:54:02Z

Hi @kdhira , thanks for the PR!

I'll have a look tomorrow.

kdhira · 2022-02-27T06:39:13Z

Thanks @kobik

Let me know if there's any questions or anything you're unsure about. Like I mentioned before, the fix minimally circumvents the NPE which gets the route label working with just 'N/A' for my usecase.

Also I'm hoping that these changes can be released to NPM as a version of prometheus-api-metrics, probably something like 3.2.1 of the package. Is there anything extra I need to do (ie version bumping etc) or is that all handled by you?

kobik

Thanks @kdhira.
Is it possible to add a test for this scenario?

Issue: PayU#59

kdhira · 2022-03-01T21:32:49Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "prometheus-api-metrics",
-  "version": "0.0.0",
+  "version": "3.2.0",


I found this to be the problem with package-lock.json file, giving me a version difference. I restored the 3.2.0 version to the package.json

Also I had to use Node v15 with the v2 lockfileversion otherwise it would try to restore the v1 config, rewriting most of the package-lock.json

the version should remain 0.0.0 as we use semantic-release to generate the next version which sets it in the package.json file as part of the release process

Ah okay, thanks for explaining this. This is easy for me to revert, will switch it back

I have reverted the package version to 0.0.0

kdhira · 2022-03-01T21:35:02Z

test/unit-test/metric-middleware-koa-test.js

@@ -653,6 +653,57 @@ describe('metrics-middleware', () => {
                Prometheus.register.clear();
            });
        });
+        describe('when using middleware request and path not matched to any specific route', () => {


I had to spend a little time to understand how the tests were made and what level of things were being tested. So I believe this 1 unit test is sufficient for the code changes I've made. But please advise if there is anything else you request

kdhira · 2022-03-01T21:37:00Z

Hi @kobik,

I added a unit test for this scenario. Please let me know if anything is wrong. I also have a commit for updating dependencies from npm audit fix.

kobik

@kdhira thanks for your PR.

See my comment

kdhira · 2022-03-08T07:30:14Z

Thanks @kobik,

I've seen your comment regarding the package version, and have reverted it. I had this as a separate commit so it was simple for me to just rebase it out.

Let me know if there's anything else I should change :)

kobik · 2022-03-08T07:54:32Z

@kdhira, can you also revert the package-lock.json changes?

I'd like to handle them in a separate PR.

kdhira · 2022-03-08T07:59:26Z

@kobik, I have dropped my commit that updated dependencies (and doing so reverted the package-lock.json changes I made). Hope this helps!

kobik · 2022-03-08T08:14:07Z

Sure, thanks again @kdhira !

kobik · 2022-03-08T08:20:19Z

@kdhira it seems like the new test didn't cover our case.

see https://coveralls.io/builds/47157280/source?filename=src%2Fkoa-middleware.js#L99

kdhira · 2022-03-08T08:41:42Z

Hmm I'll have to look at the test again, but I'm pretty confident that line gets run as the test I made has to resolve the route to use in the metric as 'N/A' somehow (through the || 'N/A' in _handleResponse) ...

kdhira · 2022-03-08T08:45:27Z

Does the coveralls tool run the unit-tests or the integration tests (or both) to detect coverage?

Issue: PayU#59

kdhira · 2022-03-08T11:23:54Z

Hey @kobik,

I think I have done what I need to ensure coverage. I added an integration test, and updated the test koa-server implementation so that the wildcard endpoint case could be correctly tested.

I also ran npm run integration-tests and after this addition it no longer shows line 99 as being an Uncovered Line.

Should be ready to run the Github workflows again

kobik · 2022-03-08T11:30:40Z

you're correct, for some reason it's only collected from integration tests.
you can update the workflow to run nyc npm test instead of running unit and integration separately, which should collect the coverage of both of them.

but also what you did should be fine 👍

github-actions · 2022-03-08T13:10:48Z

🎉 This PR is included in version 3.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Check if properRoute is found before acting on it

3d4c631

In the cases where the request route is not matched to any Koa route, bail out before trying to format the path string. Issue: PayU#59

kdhira mentioned this pull request Feb 21, 2022

Development still active? #72

Closed

kdhira commented Feb 21, 2022

View reviewed changes

kobik requested changes Feb 28, 2022

View reviewed changes

Add test for koa middleware unmatched route scenario

1ec335e

Issue: PayU#59

kdhira commented Mar 1, 2022

View reviewed changes

kobik requested changes Mar 8, 2022

View reviewed changes

kdhira force-pushed the fix/#59-catch-unmatched-routes branch from d99901e to f1c554f Compare March 8, 2022 07:28

kdhira force-pushed the fix/#59-catch-unmatched-routes branch from f1c554f to 1ec335e Compare March 8, 2022 07:58

kobik previously approved these changes Mar 8, 2022

View reviewed changes

Add integration test for wildcard path scenario

b59aa57

Issue: PayU#59

kdhira dismissed kobik’s stale review via b59aa57 March 8, 2022 11:20

kobik approved these changes Mar 8, 2022

View reviewed changes

kobik merged commit d11c383 into PayU:master Mar 8, 2022

github-actions bot added the released label Mar 8, 2022

kdhira deleted the fix/#59-catch-unmatched-routes branch March 8, 2022 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if properRoute is found before acting on it #73

Check if properRoute is found before acting on it #73

kdhira commented Feb 21, 2022

kdhira Feb 21, 2022

kdhira commented Feb 26, 2022

kobik commented Feb 26, 2022

kdhira commented Feb 27, 2022

kobik left a comment

kdhira Mar 1, 2022

kobik Mar 8, 2022 •

edited

Loading

kdhira Mar 8, 2022

kdhira Mar 8, 2022

kdhira Mar 1, 2022

kdhira commented Mar 1, 2022 •

edited

Loading

kobik left a comment

kdhira commented Mar 8, 2022

kobik commented Mar 8, 2022

kdhira commented Mar 8, 2022

kobik commented Mar 8, 2022

kobik commented Mar 8, 2022

kdhira commented Mar 8, 2022

kdhira commented Mar 8, 2022

kdhira commented Mar 8, 2022

kobik commented Mar 8, 2022

github-actions bot commented Mar 8, 2022

Check if properRoute is found before acting on it #73

Check if properRoute is found before acting on it #73

Conversation

kdhira commented Feb 21, 2022

kdhira Feb 21, 2022

Choose a reason for hiding this comment

kdhira commented Feb 26, 2022

kobik commented Feb 26, 2022

kdhira commented Feb 27, 2022

kobik left a comment

Choose a reason for hiding this comment

kdhira Mar 1, 2022

Choose a reason for hiding this comment

kobik Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

kdhira Mar 8, 2022

Choose a reason for hiding this comment

kdhira Mar 8, 2022

Choose a reason for hiding this comment

kdhira Mar 1, 2022

Choose a reason for hiding this comment

kdhira commented Mar 1, 2022 • edited Loading

kobik left a comment

Choose a reason for hiding this comment

kdhira commented Mar 8, 2022

kobik commented Mar 8, 2022

kdhira commented Mar 8, 2022

kobik commented Mar 8, 2022

kobik commented Mar 8, 2022

kdhira commented Mar 8, 2022

kdhira commented Mar 8, 2022

kdhira commented Mar 8, 2022

kobik commented Mar 8, 2022

github-actions bot commented Mar 8, 2022

kobik Mar 8, 2022 •

edited

Loading

kdhira commented Mar 1, 2022 •

edited

Loading