Validation and hooks order

[PhantomYdn]

Guys,

I just found out that hooks applied to a document after validation of that document.
It seems to me, that's a design gap:

1. There are might be hooks for making an invalid document valid:
2. Hooks might change document in incompatible way

Some examples for p.1:
a) Field is mandatory and readonly and no value specified on create: hook might set default one.
b) There is an autogenerated field which should not be null - hook migh generate value

What do you think?

[andrii0lomakin]

Ilya, I agree :-)

[PhantomYdn]

Andrey,

Thank you for support:)

Guys,

Let me propose a testcase.
Right now it's failing, but if you uncomment 3 "magic lines" - it will be OK.

    @Test
    public void testValidationAndHooksOrder()
    {
        ODatabaseDocument db = wicket.getTester().getDatabase();
        OSchema schema = db.getMetadata().getSchema();
        OClass classA = schema.createClass(TEST_VALIDATION_AND_HOOKS_CLASS);
        classA.createProperty("property1", OType.STRING).setNotNull(true);
        classA.createProperty("property2", OType.STRING).setReadonly(true);
        classA.createProperty("property3", OType.STRING).setMandatory(true);
        db.registerHook(new ODocumentHookAbstract() {
            @Override
            public RESULT onRecordBeforeCreate(ODocument doc) {
                doc.field("property1", "value1-create");
                doc.field("property2", "value2-create");
                doc.field("property3", "value3-create");
                return RESULT.RECORD_CHANGED;
            }

            @Override
            public RESULT onRecordBeforeUpdate(ODocument doc) {
                doc.undo("property2");
                if(doc.field("property3")==null) doc.field("property3", "value3-update");
                return RESULT.RECORD_CHANGED;
            }

            @Override
            public DISTRIBUTED_EXECUTION_MODE getDistributedExecutionMode() {
                return DISTRIBUTED_EXECUTION_MODE.SOURCE_NODE;
            }
        });
        ODocument doc = new ODocument(classA);
        //doc.field("property3", "value3-create"); //Magic line #1
        doc.save();
        assertEquals("value1-create", doc.field("property1"));
        assertEquals("value2-create", doc.field("property2"));
        assertEquals("value3-create", doc.field("property3"));

        doc.field("property1", "value1-update");
        doc.field("property2", "value2-update");
        doc.removeField("property3");
        //doc.undo("property2"); //Magic line #2
        //doc.field("property3", "value3-update"); //Magic line #3
        doc.save();
        assertEquals("value1-update", doc.field("property1"));
        assertEquals("value2-create", doc.field("property2"));
        assertEquals("value3-update", doc.field("property3"));
    }

P.S. Btw, one more related issue: it's possible to create an document with property=null even if that property marked as "not null". Please let me know if separate issue should be created for this as well.

[PhantomYdn]

@tglman , @lvca, @Laa ,

Are you still planning this for 2.1? I'm asking because I understand: it's big, but at the same time is very important for proper use OrientDB in real use cases.

[tglman]

I think i can add a "double" validation only in case an hook changed the record, this should fix the problem, isn't ?

[tglman]

Nope your test case show that the validation block the flow before reach the hook, so you need it to be executed only after the hook call, i'm not sure how safe is to do that now, it can create problem in other cases, for example it can let some hook implemetation crash because before where protected by the validation and after this change will be not.

by the way @PhantomYdn have you seen the new property "default value" this may solve your problem and this can be left as it is.

anyway i think exectute the validation also after the hook is called in case the hook changed something is a good practice. i'ill implemet that.

[PhantomYdn]

Lets split this question to several ones:

1. How must it be from logical stand point?

2. How to make movement smooth?

3. Imho, the only logical way: execution of validation always after hooks and before storing in DB. There are several mentioned reasons for that above. Do you agree?

4. That's a big problem: I understand... As a workaround might be the following approch be used: hooks should have one more configuration parameter: where to execute them - before validation, after validation. Default will be "after validation". Also it will be cool reflect some how: is this should be executed just "once per event", or "everytime on change". For example we might have hook to log a record and it's important to log it "as is" before any hooks. What do you think?

"default value" is not solving the whole set of problems there...

[PhantomYdn]

Btw: one more idea: validation might be done like a hook. And of something should be executed before the validation - it should have approapriate order.

[lvca]

@tglman This is related to your issue about validation?

[PhantomYdn]

Guys, we really want to see this in 2.1. Please let me know:

1. when do you expect release 2.1 GA?
2. how many RC do you expect prior to 2.1 GA?
3. Do you mind if I will try to implement this? But impact might be huge! That's why RC will be desired...

[tglman]

hi @PhantomYdn

for the release process i let @lvca give more details.

In the design part i see the point, i was actually wondering if could have mean add a new "hook event" beforeValidate, i'm not a huge fun of hooks, and more hooks we add slower we get, but there is the use case so let's do it, feel free to implement it, and we are also happy to accept directly your own solution :)

Thank you for the huge contributions you do often :)

[lvca]

@PhantomYdn I've no objection on this. Moving validation as hooks would allow your special behavior. For me it's a low priority. WDYT about providing a PR about that? If all tests pass and somebody else don't see any objection we could merge it asap.

[PhantomYdn]

Thanks guys! Once it will be implemented I will send a PR.

[lvca]

👍

[lvca]

@PhantomYdn Any news on this PR?

[PhantomYdn]

Focused on other things for a while... How many days do I have for this one to complete development?

[lvca]

OrientDB 2.1 GA is scheduled for the end of the next week, but we'd need time to try this PR avoiding to put unstable things at last minute. We could also move this to 2.2.

[tglman]

quite close to the final release moving to 2.2