complex types should be constructed with arguments

Bug #1220906 reported by Doug Hellmann
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
WSME
New
Undecided
Unassigned

Bug Description

Complex and user-defined types are constructed by calling __init__() without any arguments and then calling setattr() to set the attributes. This doesn't let the instance do any validation across attributes, to check for things like mutually exclusive settings or combinations of values that do not make sense.

https://github.com/stackforge/wsme/blob/master/wsme/rest/args.py#L69

Revision history for this message
Christophe de Vienne (cdevienne) wrote :

I am not so sure using the default constructor is the right way to go.

We may want to keep different behavior when a complex type is received and when we init it for sending.
Moreover, for now at least, we cannot assume the constructor has parameters for each exposed attribute, nor that they have the same names.

May be a specialized factory function would be better, that could be overloaded for additionnal checking.

Revision history for this message
Doug Hellmann (doug-hellmann) wrote :

How about if __init__() is used for values coming from the web, and class methods are used for creating instances in the code for special cases? That would be consistent with the sample() method API, for example.

We could say that the arguments to __init__() need to match the attributes, and the base class could take **kwds and filter out anything that isn't an attribute before using setattr() to set the value. That preserves the current behavior, but allows a subclass to define constraints in __init__() before calling super().

Revision history for this message
Christophe de Vienne (cdevienne) wrote :

The user should be able to freely override the constructor. Putting constraints on its parameters will not bring a lot, and will definitely produce errors when people attempt for change its behavior. The only reason I added a default __init__ on Base is to make is easier and more natural to manipulate the complex types from the code.

When you suggest class methods are used for creating instances for special cases, I agree. I also think initialising a complex type with values from the web _is_ a special case.

Ideally we should be able not to put _any_ constraints on the __init__ arguments, not even the one we currently have which is to be callable with no arguments. I know it is doable, because the SQLAlchemy mapped classes behave this way : __init__ is not called when objects are loaded from the database. IIRC it is achieved by playing around with __new__.

To conclude, I would be in favor of a special class method for reading from the web, and a freely customizable __init__.

Revision history for this message
Doug Hellmann (doug-hellmann) wrote :

I didn't know SQLA worked that way. I'll look at how that works.

Your point on treating instantiating the object from the web as a special case is interesting. I'm not sure that it makes sense to say there are no constraints on the constructor, though. It seems like the two main use cases are instantiating the object from the web, and instantiating the object to return it. In the latter, the caller can control how the instantiation happens by choosing which method to call explicitly, so calling a class method makes sense. But coming in from the web, we can really only call one method. Perhaps adding a new class method with a well-defined name would work for that, but it also seems like just using __init__ makes sense.

I'll have to give this some more thought.

Revision history for this message
Christophe de Vienne (cdevienne) wrote :

Well, my main idea is that the complex types should be like regular classes as much as possible. The same underlying idea of SA classes being only serialised to DB (which I do realise is not the same case so the comparison has limits).

But my point of view may be excessively influenced by my habits and the way I use the complex types right now. And I know that such change on __init__ behavior will definitely break code.

I also have the feeling that what you seek is actually a better validation system that allow arbitrary consistency checks.

So a question is : should these validations be the sames when data are coming from inside and outside? Note that the validations that are done currently are always the same, but when we start speaking of more arbitrary checks, would it make sens to know the data origin?

I think this question should be included in the rethinking of complex types.

Revision history for this message
Doug Hellmann (doug-hellmann) wrote :

Clearly we want strict validation for data coming from the API-side, but I don't see the harm of having that same validation for data coming from the inside (like a database or something). Maybe I'm missing a use case?

Revision history for this message
Christophe de Vienne (cdevienne) wrote : Re: [Bug 1220906] Re: complex types should be constructed with arguments

No harm in having the same validation for data from inside, on the
contrary. The only difference is the meaning of the raised error : a
user error or a failed assertion.

But this is not the use case I have in mind.

The thing is that the data from inside may come in a very different form.
For example the complex types I use to map a SQLAlchemy model (see
wsmeext.sqlalchemy).

From "inside", we want to initialise the complextype from a SQLAlchemy
object, and only that.
So the constructor will look like :
def __init__(self, sa_instance):

If the constructor is used to load datas from outside we're doomed...

The solution would be to have a special factory function for creating
objects from a sqlalchemy instance. It is a perfectly valid solution,
except not the one I used in my existing code -> which is why I said
that I may be influenced by my existing code. It also have the
inconvenient to be more confusing, which is a highly personal point of view.

Revision history for this message
Doug Hellmann (doug-hellmann) wrote :

OK, I understand now. We use class methods as factories in Ceilometer, passing them "internal" objects and getting back an API object. That works well for us.

I did find today that some code paths in WSME look for a separate validate() method and call it when it is found, so it's possible that we can already do some of the validation that I want using that hook.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.