Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

the design seems to be backward the django-rest-framework serializers. #278

Closed
hackrole opened this issue Feb 11, 2015 · 5 comments
Closed

Comments

@hackrole
Copy link

I go throught some features of the both. and below was my opinion:

  1. the validate raise error is not good. the django-rest-serializers return true or false, and store the errors in the models. this is more usable.

  2. cannot serializers more than one field. the django-rest-serializers can use like below:

users = UserModel([{'uid': '111', 'mobile': '111'}, {'uid': '222', 'mobile': '222'}], many=True)

but the schematics cannot do. indeed, many ORM-lib will support both single-and-many objects. for example: the mongoengine with queryset.
3) you can call to_native() even the validate return errors.

try:
     models.Model.validate():
except Exception as e:
     print e.message

models.to_native()
  1. the serializable and roles design was really wonderful, anyway.
@fabiosussetto
Copy link

Don't agree with 1). I prefer validation to raise an exception, containing the error messages. I think storing the errors in the model it's more awkward and less flexible. I don't want an external library to mess around with my data structures. What's wrong with an exception?

Not really sure if 2) is needed as well, I mean, it's just trivial to implement that functionality yourself without bloating the API with extra flags.

Just IMO.

@bintoro
Copy link
Member

bintoro commented Feb 24, 2015

Yeah, we're open to any ideas, but in cases such as these, I would like to see some concrete examples that illustrate why the alternative is better. @hackrole, why would you store errors on the model? What's the practical benefit?

And about this:

users = UserModel([{'uid': '111', 'mobile': '111'}, {'uid': '222', 'mobile': '222'}], many=True)

What value does users get here? Is it a list? Is it some custom wrapper that represents a list of model instances? What's the upside compared to the following?

users = [UserModel({'uid': '111', 'mobile': '111'}), UserModel({'uid': '222', 'mobile': '222'})]

@hackrole
Copy link
Author

@bintoro,
first, I think store errors on the model object instead of raise a Exception is really a better design.
Exception is more rough, and will break normal flow. And I found I would like not to raise Exception at most situation. for example:

  1. a rest-api for user to update his sex, but the client give the wrong value. I would not return a 500 error, but raise 400 bad-request.
# exception
try:
     models.validate()
except Exception as e:
     raise Exception('http 400 %s' % e.message)

# not raise 
if not models.validate():
      raise Exception('http 400 %s' % models.errors)

the later is more clean.

  1. some times, after validate() errors, I may want to log the error msg and do some clean. but the only way to get the error msg is catch the exception. and I have to do log and clean on the except block.

  2. if really need, I would like raise Exception myself, I may raise Http400, http401, http500 and so on. but the code raise validate-exception, which usually case a http500 errors. I may need catch a exception and reraise another exception.

@hackrole
Copy link
Author

@bintoro
second, I mean the models should support both single model-object or a model-object-list. like the mongoengine have both model-class and queryset.
I know you can use list as a replacement of the queryset. so you may get the follow code.

# init from db-query or client request
users = [UserSchematics(**user) for user in user_list]
# validate data
for user in users:
    try:
         user.validate()
    except Exception:
          pass
# render to json
data = [user.to_native() for user in users]
json.dumps(data)

but if you can support queryset like use many or other ways, the code may look like:

# init from db-query or client-request
users = UserModel(user_list, many=True)
# validate
if not users.validate(): print users.errors
# render 
json.dumps(users.to_native(many=True))

@bintoro
Copy link
Member

bintoro commented Nov 5, 2015

(1) Errors inside models instead of exceptions: Sorry but this is unlikely to happen, as it seems extremely complicated if you have a tree of models referencing other models. An exception provides all the errors in one place.

(2) Queryset-like multi-instances: I think this sort of functionality should be provided by an extraneous wrapper class, not the Model class itself. A PR implementing this would definitely be evaluated.

@bintoro bintoro closed this as completed Nov 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants