Use static model factory methods

[delenius]

Fixes #2113

The constructFromObject factory methods should be class methods
(or "static" methods), not instance methods.

With this commit, ApiClient no longer calls the model constructors
directly. Instead, it calls the new static factory method to get the
new instance. If there is no data on the top level, null is returned.

It is still possible for users to call the model constructors
directly, of course.

[demonfiddler]

I don't think this was the right thing to do. It seems to me that that in order to support composition (via the allOf keyword) it has to be possible for an instance of the composed class to call the constructFromObject() methods from all of the classes of which it is composed, in order that they can each copy their appropriate property values across. Converting this method to a static that always returns a newly created object defeats this important use case, unless the template replicates the property-copying code from all the composed classes, which is wasteful, inefficient and entirely unnecessary.

Naturally, there needs to be some logic somewhere to decide which class to instantiate, and at present this resides in the API classes, as each operation knows what its return type is.

Also, I've see discussion elsewhere about polymorphism and the use of a discriminator field to determine the runtime type of the sub-class to instantiate, and by the way in my view there ought to be a default implementation based on, say, an implicit $type property whose value is (say) an expression that evaluates to the sub-class's constructor. This would obviate the need to explicitly incorporate such runtime type metadata in the domain schema (it doesn't belong there, in most cases). The precedent already exists in many OO systems. For example, look at XML Schema's 'xsi:type' attribute, which stores the namespace URI of the complex type. Look at Java serialization - deserializing a serialized subclass instance automatically returns an identical object without your having explicitly to model a mechanism to store the type name.

[delenius]

Oh, do we have multiple inheritance in Swagger specs? That might be a mess to implement in all the single-inheritance languages...

[demonfiddler]

The allOf keyword supports a list of the composed classes. So sure, we have at least multiple composition, which in a duck-typed language like JavaScript amounts to something close to multiple inheritance. It isn't instance-level composition either, because it's not delegating to composed instances. But then even single inheritance isn't realised unless the templates actually set up the correct JavaScript prototype chains, which right now they don't seem to do. Consequently, the templated prototype getter and setter methods only work for the owned properties and not for the inherited properties, which is another problem.

BTW in other modelling systems that support multiple inheritance (e.g., MOF, UML, EMF), realisations in single-inheritance languages (e.g., Java) are achieved by extending the first superclass and re-implementing everything that's inherited (in Java via interfaces) from the second and subsequent superclasses.

Strikes me there's a good deal of thinking & careful design work needed before the JS codegen will be complete and fit for real-world use. BTW I've had decades of experience in modelling and code generation and I'm happy to advise. As for making actual code contributions I'd have to obtain my boss's consent - I've done it on other open source projects.

[delenius]

Yeah, well, that all is a much larger discussion than just for the JS client then. Also, I don't see how the previous version of constructFromObject supported this type of composition. It was an instance method, meaning you could not call several different constructFromObject version from different classes.

It seems to me that you would need those methods to be static (as they are now), but to add an argument where you pass the object into those methods. For example, if class C is composed of classes D and E,

var obj = new C();
D.constructFromObject(obj,data);
E.constructFromObject(obj,data);
...

This is actually closer to what we have now than to what we had before. But I won't be submitting that PR ;)

[demonfiddler]

I've just realised that the previous implementation of constructFromObject was replicated on each and every instance, which was unnecessary - could easily be defined on the prototype, in which case one could invoke for obj thus:

var obj = new C();
obj.constructFromObject(data);

C.protoype.constructFromObject = function(data) {
  // Copy inherited properties.
  D.prototype.constructFromObject.call(this, data);
  E.prototype.constructFromObject.call(this, data);

  // Copy own properties.
  this['x'] = data['x'];
  ...
}

At any rate, something along these lines is called for.

P.S. I've just noticed that the Java codegen only processes the first supertype in the allOf array. Sigh...

[delenius]

There is also the issue of where to set the default values, if any, and making that work both when objects are constructed using the constructor directly, and when using the constructFromObject method.

Also, I wonder if the semantics of composition has been fully worked out. For example, what happens if I compose two classes where

• Both have the same property, but with different types
• Both have the same property, but with different default values

I'm sure there are other such issues to consider.

[fehguy]

@delenius, @webron may have more thoughts on this but I believe the intention of defaultValue is that the server will add that value, not the client. So I don't think there's value in adding it to the generated code.

[webron]

@delenius in most cases the default value is indeed more useful to the server side, but it can apply to the client as well. Hopefully http://swagger.io/unlocking-the-spec-the-default-keyword/ sheds more light on it.

As for composition/inheritance, it is very easy to create definitions that either don't make sense (like different default values for the same property, when coming from inheritance). Those are either due to user error or limitations of JSON Schema. The best (and probably only) way to deal with those is to simply assume that the first/last definition is the 'winner' and that's that. There's no clean solution to it.

[delenius]

@fehguy your comment may be relevant to #2143
@wing328 told me we need these things. Note that we are talking about defaults on the model classes, not defaults directly on the operation parameters.

[wing328]

@delenius yes, I was referring to default value in model's properties.. We don't implement default value in the parameters as that should be better implemented in the server side. The same has been done in other API client (C#, Ruby, PHP, Python, etc).

There are use cases in which only models are needed/generated from the Swagger spec (using -Dmodels)

[demonfiddler]

Regarding default parameter values, I concur with the view that the client should not need to pass them and that the server should infer them.

Regarding default property values: since these are modelled in the YAML, the expectation is that an instance will have the specified default property value from the outset. There are two ways to achieve this:

1. Set the default value explicitly in the constructor, thus creating an owned property. The disadvantage of this approach is that owned properties with the default value will be unnecessarily serialized to JSON when crossing the wire, making the payload larger than it needs to be.
2. Add a prototype property with the default value, in which case the property won't be an owned property of the instance until/unless it is set elsewhere (in client code, for example).

In either case, the constructFromObject implementation would need to be smart enough to avoid explicitly setting instance properties to the value on the data object if the data object does not actually have an own property of that name. For example:

C.protoype.constructFromObject = function(data) {
  // Copy inherited properties.
  D.prototype.constructFromObject.call(this, data);
  E.prototype.constructFromObject.call(this, data);

  // Copy own properties.
  for (prop in C.protoype) {
    if (data.hasOwnProperty(prop))
      this[prop] = data[prop];
  }
}

Also in either case, a mechanism to unset a named property back to its default value is required. In approach 1 above this could be implemented as a static method of the class but the implementation would be complex as it would have to account for inheritance/composition. In approach 2 this could be achieved by the simple expedient of deleting the property. Or we could provide a global static helper method that accepts the instance and property name(s) as parameters - it would delete the instance property(ies) and thus further references would be resolved via the prototype chain for properties inherited from the first allOf type; properties 'inherited' from the second or subsequent allOf types would need to be duplicated on the prototype. Overall I think this is the most satisfactory approach.

I've seen various assertions here and in the Swagger docs that allOf entails type composition not inheritance. Yet based on the presence of a boolean io.swagger.codegen.DefaultCodegen.supportsInheritance field there seems to be at least the intention of supporting inheritance. It would seem churlish not to exploit the target language's extension mechanism even if it is only single inheritance. Also, the JavaScript model.mustache emits a comment to the effect that the class extends the {{parent}} model, although right now I'm trying to understand why parent ends up set to the first allOf $ref in some cases to the last in other cases.

Incidentally, in some languages (e.g., Java) there's 1.) no way to delete a property and 2.) no way for primitive values to be 'unset' since JVM defaults always apply (e.g. boolean: false, numeric: 0). JavaScript has no such limitations.

Regarding composition semantics, I see two ways to resolve conflicts. The first approach is to have the validator reject any specification that contains conflicting composed properties (i.e., abort codegen). The second approach would simply allow successive definitions to override prior definitions, by post-order traversal of the allOf inheritance tree. Or one could make the behaviour a generator option (strict vs. lenient). There may be other validations that one could control via this option.