Add a callback handler to check incoming payload data before writing. about silverstripe-restfulapi

[colymba]

I think that's a great idea. :)

I would however have the 2 callbacks onBeforeDeserialize and onAfterDeserialize on lines 388 and 394 so devs can act on both the raw json and the formatted data array that will be applied to the DO. Remembering to pass the payload by reference each time...

I think that would give the right kind of flexibility?

[bummzack]

I'm not sure if the raw request-body would be of much interest? But more flexibility is better, I guess.

Your proposal sounds good as well.. as long as there's a way to check/sanitize incoming data before writing, I'll be happy :)

[bummzack]

I thought about this some more and wondered if it would be possible to somehow also perform a callback/precondition before creating records?

Imagine the following use-case: There are Queues, every Queue has_many Tickets. The amount of tickets per queue is restricted to 10.

How would you go about implementing this? Implementing a canCreate method on Ticket won't do, because it doesn't know about the queue it's going to be added to.

Your implementation checks canCreate, then creates the record and after that runs an update. If there's a hook in updateModel as discussed above (where we could prevent something), the DO will already be persisted to the DB, just without the actual data from the payload.

Maybe this is also an edge case and would be better solved by subclassing RESTfulAPI_DefaultQueryHandler and overriding createModel?

[bummzack]

Should I take a stab at this?

My proposed changes would include the following:

• Add a onBeforeCreateWithPayload callback that allows implementing classes to deny creation of an object by looking at the payload.
• Add onBeforeDeserialize and onAfterDeserialize as discussed before... although I doubt that the incoming JSON string is of much interest (needs to be parsed anyway to do something meaningful). So this could be simplified to something like onBeforeUpdateWithPayload.

[colymba]

I think onBefore/AfterDeserialize are enough.

The before one, even if not always useful, let you alter the JSON and so how it will be parsed, e.g. many_many_extrafields but also anything else that might be implemented in the future.

The after one is great to alter what is to be written to the DO.

For denying creation of DO I don't think this is the right place. This can be done with existing SS logic already, like onBeforeWrite, canCreate and here DO validation would do the trick.

[bummzack]

You're right, this could be covered with existing SS logic, but would require a rewrite of your code.

As far as I can tell, your code that creates a new DO currently does this (simplified):

1. Check canCreate on a singleton instance. If allowed, then:
2. Create a new Object instance and write it to the DB.
3. Populate the object via updateModel. Access permissions are checked again, but before actually applying any payload data.

So both these canCreate checks won't help with the use-case I mentioned (Ticket Queues with max. amount per Queue).

In order for this to work, the creation of an Object should go like this:

1. (optional) Check canCreate on a singleton instance to exit early.
2. Create an object instance and populate it with the DO payload (only DO values without the relation data)
3. Check canCreate. If granted, write the object.

onBeforeWrite isn't meant to be used to cancel object creation or object-writing in general.

[colymba]

DO validation is what should be used here I believe. And it's true that at the moment it would not work, but solution have been discussed here #30 and here #22. And that should be implemented soon.

[bummzack]

Hm. I didn't even know that validation for DataObjects exist.. but you're right, it would make a lot of sense here.

Do you need a hand implementing this? I'd rather have this feature sooner than later.

[colymba]

I'll get the refactoring of create/updateModel done early this week so DO validation works, since I have a good idea on how to implement this.

If you can submit a PR for the onBefore/AfterDeserialize hooks that would be helpful.

[bummzack]

Awesome. Will send a PR for the deserialize hooks shortly.

[bummzack]

PR is sent. #46