Add missing methods that help evaluate if given collection is empty or not

[derberg]

Reason/Context

As discussed here #85 (comment) we need more functions like hasMessages or hasSchemas in the components model, but it would be also nice to analyze other places.

Description

Add to components model functions like hasMessages, hasSchemas, hasSecuritySchemas

[magicmatatjahu]

@derberg @fmvilas Could I handle that issue? I think that also methods like .info().hasContact(), .server({server Name}).hasDescription() etc. would be helpful for optional fields.

[derberg]

@magicmatatjahu go for it!

[magicmatatjahu]

I started looking at the code and noticed that collections are often returned as empty objects or empty arrays, even if the field is not defined, e.g. tags field, like here https://github.com/asyncapi/parser-js/blob/master/lib/models/asyncapi.js#L136 In my opinion it should return null if field is not defined, and also in case if collection (array or object) is empty (zero indexes in array and zero keys in object). For primitives, like string, numbers etc. if field is defined and empty (only for empty string) the method should return null, otherwise value. I don't know what impact it will have on e.g. html template, with a change from an empty array to null, but I could improve it everywhere.

Also I tried apply docs for extensions method by using mixins:

const extensionError = 'Extension key is not prefixed. Please ensure you prefix it with `x-`.';

/**
 * Implements functions to deal with the SpecificationExtensions object.
 * @mixin MixinSpecificationExtensions
 */
const MixinSpecificationExtensions = {
  /**
   * @returns {boolean}
   */
  hasExtensions() {
    return !!this.extensions();
  },

  /**
   * @returns {(Object<string, Any>|null)}
   */
  extensions() {
    const result = {};
    Object.entries(this._json).forEach(([key, value]) => {
      if ((/^x-[\w\d\.\-\_]+$/).test(key)) {
        result[String(key)] = value;
      }
    });
    return Object.keys(result).length ? result : null;
  },

  /**
   * @param {string} key - Extension key. Must start with `x-` prefix. Otherwise function throw an error.
   * @returns {Any}
   */
  extension(key) {
    if (!key.startsWith('x-')) {
      throw new Error(extensionError);
    };
    return this._json[String(key)];
  },

  /**
   * @param {string} key - Extension key. Must start with `x-` prefix. Otherwise throw an error.
   * @returns {Any}
   */
  ext(key) {
    return this.extension(key);
  },
};

module.exports = MixinSpecificationExtensions;

// Some model
const { mix } = require('../utils');

const Base = require('./base');

const MixinSpecificationExtensions = require('../mixins/specification-extensions');

/**
 * Implements functions to deal with the License object.
 * @class License
 * @extends Base
 * @mixes MixinSpecificationExtensions
 * @returns {License}
 */
class License extends Base {
  /**
   * @returns {string}
   */
  name() {
    return this._json.name;
  }
  
  /**
   * @returns {boolean}
   */
  hasUrl() {
    return !!this._json.url;
  }

  /**
   * @returns {(string|null)}
   */
  url() {
    return this._json.url || null;
  }
}

module.exports = mix(License, MixinSpecificationExtensions);

And as you can see, it works!

I created mixins for common types in models like description, externalDocs, bindings, tags and above extensions.

I could make PR on the next week, but I won't hide it, it will be quite big for review, because I found a lot of errors and by adding missing tests, turned out that some things throw exceptions. Divide it into two or three or rather leave it all in one?

[derberg]

Good investigation, I like the idea of mixin and MixinSpecificationExtensions composition. I would have many questions like:

• why throw new Error(extensionError);
• how mix function looks like

PR would be great but as small as possible. We should not mix this PR with subject of null vs empty as it is not related.

Regarding what you saw, that sometimes instead of null and empty array is returned, I saw it in few other places and tbh I do not know why we sometimes do it. And in places where createMapOfType is used we return empty object.
@fmvilas would have to share more details.

I know common practice is to return null but tbh I'm not a fan, I prefer empty because then I do not have to add ifs and test them explicitly. I prefer empty and those helper functions like has so I can use them when I really have to make a condition. But I might be the only one 😄 Thanks to the approach with empty I don't think we even need such ifs really https://github.com/asyncapi/html-template/blob/19a47fefb78944e6b83e7be5cd52ba75a8ab3dc0/partials/servers.html#L29

@jonaslagoni you wrote some code for templates, what is your opinion here?

[jonaslagoni]

I think empty arrays from a templaters point of view is better, because we can directly use the variable without checking it as you say @derberg. The less code in the templates the better 😄

But I can understand why null would be prefered in other scenarios.

[magicmatatjahu]

@derberg

• I throw an exception, because as we can see in the function body, we can get from it to other elements from the _json object, which should not take place. Instead of error, I can return null in this case, no problem.
• mix function is very small and looks like:

utils.mix = (clazz, ...mixins) => {
  mixins.forEach(mixin => Object.assign(clazz.prototype, mixin));
  return clazz;
}

and then we also must use @mixes jsdoc annotation for class and explicit define @class {className}. Without that last, jsdoc has a problem with checking methods and duplicates functions from mixins.

I can consolidate all models with the null problem in another PR, but we must agree on how it should look. At the moment, what you wrote guys:

• empty array or undefined array -> return []
• empty object or undefined object -> return {}

But what with primitives and custom types? My suggestions:

• undefined or empty string ('') -> return null, otherwise value.
• another primitives, if undefined return null, otherwise value.
• custom model like Tag etc, if index in array or key in object is undefined, return null, otherwise return instance of model.

What do you think?

Btw. Thanks for the activity in the issue :)

[fmvilas]

For me, the thing about returning empty arrays, returning null, or throwing an exception has more to do with the context than doing something as a general rule of thumb. I'd consider:

1. Does undefined (the key is not present) mean something different than an empty array?
2. Does null (the key is present and its value is null) mean something different than an empty array?

If, and only if, the answers to the questions above is "no", then I'll go for empty arrays for better UX. Otherwise, I'll go for its raw value (undefined or null). Throwing errors is a pattern I never liked but that's a personal preference based on nothing else than taste.

And regarding adding more methods, I'd also consider adding .property(propName) to the Schema model.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 30 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

[derberg]

duplicate #173