feat: add validation of query params in channel name #191

juergenbr · 2020-10-29T13:58:33Z

Description

added validateChannelNames test suite
added validateChannelName validator
added method to check for URL parameters in string with RegEx

Related issue(s)
Fixes #145

derberg · 2020-10-29T15:46:22Z

@juergenbr looks good. 2 questions:

as you can see our linter doesn't like regex 😄 (btw, you can run in locally with npm run lint. What do you think about using URL API instead? like
```
const channelName = '/user/signedup?foo=1';
const url = new URL('http://'+channelName);
console.log(url.search) //logs ?foo=1;
```
I think it would be possible to integrate new function with validateChannelParams (of course name would have to be changed to validateChannel. Asking because this way validator would iterate over list of channels only onece, not twice 🤔 what do you think?

juergenbr · 2020-10-29T16:48:23Z

@derberg good points, yes. I thought that using a similar expression as parseUrlVariables would not be an issue, sorry.
How should a case look like when both checks fail? Should I handle them via a try catch block and output both error messages?

derberg · 2020-10-29T17:20:54Z

@juergenbr you can keep regex, I just suggested different approaches as then you do not have issues with a linter. You can also just modify your function to have a similar approach as in parseUrlVariables and that is fine too. Just do not create new regex instance, my guess it is the issue.

regarding case when 2 issues show up at the same time, don't worry about it, they do not have to be thrown at the same time. We have such a case here already https://github.com/asyncapi/parser-js/blob/master/lib/customValidators.js#L180-L196

juergenbr · 2020-10-29T17:25:38Z

@derberg oh okay, no problem I'll fix it.
Thanks for the reference, then I'll handle it similarly!

…st suites

juergenbr · 2020-10-29T20:45:20Z

Test cases now work as expected, but due to the merge the new method got a little too complex so I split part of the logic. Please let me know if this is fine or if I should find another way.

derberg

it is getting a very good shape. I added some comments, long story short -> you need to add one more test case there that I do not think is covered with current implementation

derberg · 2020-10-30T09:45:02Z

lib/utils.js

+  if (typeof str !== 'string') return;
+  const channelName = str;
+  const url = new URL(`http://${channelName}`);
+  console.log(url.search); //logs ?foo=1;


please remove this console.log

derberg · 2020-10-30T09:58:06Z

lib/customValidators.js

+  if (invalidChannelName.size) {
+    throw new ParserError({
+      type: validationError,
+      title: 'Channel names with parameters exist',


Suggested change

title: 'Channel names with parameters exist',

title: 'Channel names contain query parameters',

derberg · 2020-10-30T09:59:00Z

lib/customValidators.js

+      type: validationError,
+      title: 'Channel names with parameters exist',
+      parsedJSON,
+      validationErrors: groupValidationErrors('channels', 'channels contain invalid name with url parameters ', invalidChannelName, asyncapiYAMLorJSON, initialFormat)


Suggested change

validationErrors: groupValidationErrors('channels', 'channels contain invalid name with url parameters ', invalidChannelName, asyncapiYAMLorJSON, initialFormat)

validationErrors: groupValidationErrors('channels', 'channels contain invalid name with url query parameters', invalidChannelName, asyncapiYAMLorJSON, initialFormat)

derberg · 2020-10-30T10:03:03Z

lib/customValidators.js

+    const variables = parseUrlVariables(key);
+    const notProvidedChannelParams = notProvidedParams.get(tilde(key));
+    const parameters = parseUrlParameters(key);
+    if (!variables && !parameters)


because of the following ifs, I don't think we need this one

derberg · 2020-10-30T10:06:04Z

lib/customValidators.js

+  chnlsMap.forEach((val, key) => {
+    const variables = parseUrlVariables(key);
+    const notProvidedChannelParams = notProvidedParams.get(tilde(key));
+    const parameters = parseUrlParameters(key);


maybe rename parameters to queryParameters and parseUrlParameters to parseUrlQueryParameters?

this will make code slightly clearer. Problem with current naming is that this function validates, query params and also if variables have params object. So you see "parameters" in 2 different contexts with totally different meaning. This is why I think we need to make super clear that it is query param here

lib/customValidators.js

derberg · 2020-10-30T10:10:30Z

test/customValidators_test.js

+    const inputString = `{
+      "channels": {
+        "test/test01": {
+          "parameters": {


you are validating here a channel name, so instead of providing a parameter here, I think you could just pass empty object

derberg · 2020-10-30T10:12:26Z

test/customValidators_test.js

+    const inputString = `{
+      "channels": {
+        "/user/signedup?foo=1": {
+          "subscribe": {


same as with the other test case, I think you can easily just pass an empty channel object

derberg · 2020-10-30T10:16:38Z

lib/customValidators.js

+
+  return true;
+
+  function setNotProvidedParams(variables, val, key, notProvidedChannelParams) {


I think you can move this function outside this function, as a separate private helper function, because it has the potential to be used in other validations, just don't forget about jsdoc.

Why? because I'm pretty sure you will need to use it in query params validation too, like here

if (parameters) { invalidChannelName.set(key, parameters); }

this logic is here for handling use case where more that once the error of the same title appear during validation. In other words, I'm pretty sure the current implementation is not able to handle a case where you have 2 different channels containing query parameters. Please add such a test case and you will see what I mean.

juergenbr · 2020-10-30T15:04:46Z

@derberg thanks again for your detailed feedback. I refactored the code to pull the additional method into the utils file. I also found a way to combine exceptions from both validations so that none "overrules" the other.

Regarding the case with 2 or more channels that contain query parameters: since the code you mentioned is inside the loop this case is covered. I added a test case and it is working as expected, returning 2 validation errors inside the array and returning them with correct messages. Same works if one channel has a missing parameter and one contains a query parameter.

derberg

I added some comments regarding naming of errors and jsdoc.
Great idea with displaying errors from different validations, I like it.

What is missing:

test case for channel that has no parameter object and name of channel is user/{userId}/signedup?foo=1. So 2 different errors for the same channel
in tests, I cannot see in error information about the channel (the one that failed validation) locatin in the file. Things like endColumn and others. Notice that channel variables validaton uses tilde function to make it work

derberg · 2020-10-30T15:56:11Z

lib/customValidators.js

+  if (notProvidedParams.size || invalidChannelName.size) {
+    throw new ParserError({
+      type: validationError,
+      title: 'Channel validation failed wiht following errors',


tiny spelling mistake here, with not wiht

Suggested change

title: 'Channel validation failed wiht following errors',

title: 'Channel validation failed',

derberg · 2020-10-30T15:58:12Z

lib/customValidators.js

+
+  //combine validation errors of both checks and output them as one array
+  const parameterValidationErrors = groupValidationErrors('channels', 'channel does not have a corresponding parameter object for', notProvidedParams, asyncapiYAMLorJSON, initialFormat);
+  const nameValidationErrors = groupValidationErrors('channels', 'channels contain invalid name with url query parameters ', invalidChannelName, asyncapiYAMLorJSON, initialFormat);


Suggested change

const nameValidationErrors = groupValidationErrors('channels', 'channels contain invalid name with url query parameters ', invalidChannelName, asyncapiYAMLorJSON, initialFormat);

const nameValidationErrors = groupValidationErrors('channels', 'channel contains invalid name with query parameters ', invalidChannelName, asyncapiYAMLorJSON, initialFormat);

derberg · 2020-10-30T15:58:41Z

lib/customValidators.js

+      type: validationError,
+      title: 'Channel validation failed wiht following errors',
+      parsedJSON,
+      validationErrors: allValidationErrors //groupValidationErrors('channels', 'channel does not have a corresponding parameter object for', notProvidedParams, asyncapiYAMLorJSON, initialFormat)


please remove this comment

derberg · 2020-10-30T16:02:44Z

lib/utils.js

+ * @function setNotProvidedParams
+ * @private
+ * @param  {String} variables array of all identified URL variables in a channel name
+ * @param  {Object} val The channel object for which to identify the missing parameters


Suggested change

* @param {Object} val The channel object for which to identify the missing parameters

* @param {Object} val the channel object for which to identify the missing parameters

derberg · 2020-10-30T16:04:08Z

lib/utils.js

+ *
+ * @function setNotProvidedParams
+ * @private
+ * @param  {String} variables array of all identified URL variables in a channel name


Suggested change

* @param {String} variables array of all identified URL variables in a channel name

* @param {Array<String>} variables array of all identified URL variables in a channel name

derberg · 2020-10-30T16:07:22Z

lib/utils.js

@@ -304,3 +317,25 @@ utils.groupValidationErrors = (root, errorMessage, errorElements, asyncapiYAMLor

  return errors;
 };
+
+/**
+ * return all missing properties for a channel


Suggested change

* return all missing properties for a channel

* extend map with channel params missing corresponding param object

derberg · 2020-10-30T16:08:39Z

lib/utils.js

+ * @param  {Object} val The channel object for which to identify the missing parameters
+ * @param  {String} key the channel name. 
+ * @param  {Array<Object>} notProvidedChannelParams concatinated list of missing parameters for all channels
+ * @param  {Map} notProvidedParams result map of all missing parameters 


Suggested change

* @param {Map} notProvidedParams result map of all missing parameters

* @param {Map} notProvidedParams result map of all missing parameters extended by this function

… query param test cases

juergenbr · 2020-10-30T17:31:04Z

Sorry for the doc errors, I implemented your suggestions, added the location information and a test case to cover a channel violating both checks at the same time.

lib/customValidators.js

Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>

sonarqubecloud · 2020-11-02T10:47:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
No Duplication information

derberg

LGTM. Great work @juergenbr !

asyncapi-bot · 2020-11-02T11:25:07Z

🎉 This PR is included in version 1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

derberg · 2020-11-02T12:43:21Z

@all-contributors please add @juergenbr for code

allcontributors · 2020-11-02T12:43:32Z

@derberg

I've put up a pull request to add @juergenbr! 🎉

WaleedAshraf · 2020-11-13T01:37:58Z

This is failing with error

err { Error: Invalid URL: http:///
        at parse (/Users/waleedashraf/projects/waleed/asyncapi-validator/node_modules/@asyncapi/parser/lib/parser.js:108:11)
        at process._tickCallback (internal/process/next_tick.js:68:7)
      type: 'https://github.com/asyncapi/parser-js/unexpected-error',
      title: 'Invalid URL: http:///',
      parsedJSON:
       { asyncapi: '2.0.0',
         id: 'urn:com:slack:rtm:api',
         info: { title: 'Slack Real Time Messaging API', version: '1.0.0' },
         servers: { production: [Object] },
         channels: { '/': [Object] },
         components:
          { securitySchemes: [Object],
            schemas: [Object],
            messages: [Object] } },
      message: 'Invalid URL: http:///' } Invalid URL: http:///

Schema: https://raw.githubusercontent.com/asyncapi/asyncapi/master/examples/2.0.0/slack-rtm.yml

Because the channel name is just /

derberg · 2020-11-13T08:57:32Z

@WaleedAshraf fix provided #196

juergenbr added 3 commits October 29, 2020 13:49

added method to check for URL parameters in string

f07ace0

added validateChannelName validator

fe21e13

added validateChannelNames test suite

67d5127

juergenbr added 6 commits October 29, 2020 19:46

removed RegExp from utils, merged channel validation functions and te…

c00dcac

…st suites

using URL API in parseUrlParameters instead of RegEx

53d63d0

fixed validateChannels

8adba47

fixed validateChannels

378fa00

invalid result for new url parameter test fixed

f416f10

reduced cognitive complexity

5ce9fa6

derberg requested changes Oct 30, 2020

View reviewed changes

juergenbr added 4 commits October 30, 2020 15:02

refactored validateChannels and extracted setNotProvidedParams function

abff573

removed unecessary inputs and added new validateChannels test cases

a296b1f

added setNotProvidedParams function

604101a

removed unnecessary if inside foreach loop

c9963cf

derberg requested changes Oct 30, 2020

View reviewed changes

juergenbr added 3 commits October 30, 2020 18:10

fixed jsdoc

102c303

added tilde and fixed exception messages

23911d3

added test case for one channel with two errors and added location to…

4e1c789

… query param test cases

fixed error title in test cases

baae31e

derberg reviewed Nov 2, 2020

View reviewed changes

lib/customValidators.js Outdated Show resolved Hide resolved

derberg changed the title ~~feat: Add validation of query params in channel name~~ feat: add validation of query params in channel name Nov 2, 2020

juergenbr and others added 2 commits November 2, 2020 11:42

removed empty space in nameValidationErrors message

af25d78

Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>

removed empty space in test cases

e7fd308

derberg approved these changes Nov 2, 2020

View reviewed changes

derberg merged commit d871aff into asyncapi:master Nov 2, 2020

asyncapi-bot added the released label Nov 2, 2020

allcontributors bot mentioned this pull request Nov 2, 2020

docs: add juergenbr as a contributor #195

Merged

derberg mentioned this pull request Nov 13, 2020

fix: channels with name / fail on validation #196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add validation of query params in channel name #191

feat: add validation of query params in channel name #191

juergenbr commented Oct 29, 2020

derberg commented Oct 29, 2020

juergenbr commented Oct 29, 2020

derberg commented Oct 29, 2020

juergenbr commented Oct 29, 2020

juergenbr commented Oct 29, 2020

derberg left a comment

derberg Oct 30, 2020

derberg Oct 30, 2020

derberg Oct 30, 2020

derberg Oct 30, 2020

derberg Oct 30, 2020

derberg Oct 30, 2020

derberg Oct 30, 2020

derberg Oct 30, 2020

juergenbr commented Oct 30, 2020

derberg left a comment

derberg Oct 30, 2020

derberg Oct 30, 2020

derberg Oct 30, 2020

derberg Oct 30, 2020

derberg Oct 30, 2020

derberg Oct 30, 2020

derberg Oct 30, 2020

juergenbr commented Oct 30, 2020

sonarqubecloud bot commented Nov 2, 2020

derberg left a comment

asyncapi-bot commented Nov 2, 2020

derberg commented Nov 2, 2020

allcontributors bot commented Nov 2, 2020

WaleedAshraf commented Nov 13, 2020

derberg commented Nov 13, 2020

	title: 'Channel names with parameters exist',
	title: 'Channel names contain query parameters',

	validationErrors: groupValidationErrors('channels', 'channels contain invalid name with url parameters ', invalidChannelName, asyncapiYAMLorJSON, initialFormat)
	validationErrors: groupValidationErrors('channels', 'channels contain invalid name with url query parameters', invalidChannelName, asyncapiYAMLorJSON, initialFormat)


		return true;

		function setNotProvidedParams(variables, val, key, notProvidedChannelParams) {

	title: 'Channel validation failed wiht following errors',
	title: 'Channel validation failed',

	const nameValidationErrors = groupValidationErrors('channels', 'channels contain invalid name with url query parameters ', invalidChannelName, asyncapiYAMLorJSON, initialFormat);
	const nameValidationErrors = groupValidationErrors('channels', 'channel contains invalid name with query parameters ', invalidChannelName, asyncapiYAMLorJSON, initialFormat);

	* @param {Object} val The channel object for which to identify the missing parameters
	* @param {Object} val the channel object for which to identify the missing parameters

	* @param {String} variables array of all identified URL variables in a channel name
	* @param {Array<String>} variables array of all identified URL variables in a channel name

	* return all missing properties for a channel
	* extend map with channel params missing corresponding param object

	* @param {Map} notProvidedParams result map of all missing parameters
	* @param {Map} notProvidedParams result map of all missing parameters extended by this function

feat: add validation of query params in channel name #191

feat: add validation of query params in channel name #191

Conversation

juergenbr commented Oct 29, 2020

derberg commented Oct 29, 2020

juergenbr commented Oct 29, 2020

derberg commented Oct 29, 2020

juergenbr commented Oct 29, 2020

juergenbr commented Oct 29, 2020

derberg 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juergenbr commented Oct 30, 2020

derberg 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juergenbr commented Oct 30, 2020

sonarqubecloud bot commented Nov 2, 2020

derberg left a comment

Choose a reason for hiding this comment

asyncapi-bot commented Nov 2, 2020

derberg commented Nov 2, 2020

allcontributors bot commented Nov 2, 2020

WaleedAshraf commented Nov 13, 2020

derberg commented Nov 13, 2020