Skip to content

Commit

Permalink
wrong assignment in validateValueForKey
Browse files Browse the repository at this point in the history 
do not throw away return value of _validateValueForKey that is used e.g. in ERXPartialGenericRecord
  • Loading branch information
@darkv
darkv committed Jun 27, 2016
1 parent 813f10e commit 6665a30
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion Frameworks/Core/ERExtensions/Sources/er/extensions/eof/ERXGenericRecord.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1125,7 +1125,7 @@ public Object validateValueForKey(Object value, String key) throws NSValidation.
if (cd instanceof ERXEntityClassDescription) {
((ERXEntityClassDescription) cd).validateObjectWithUserInfo(this, value, "validateForKey." + key, key);
}
value = _validateValueForKey(value, key);
result = _validateValueForKey(value, key);
}
catch (ERXValidationException e) {
throw e;
Expand Down

15 comments on commit 6665a30

@paulhoadley
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you catch this one? And what's the purpose of _validateValueForKey() anyway, given it just returns value in the implementation here?

@darkv
Copy link
Member Author

@darkv darkv commented on 6665a30 Jun 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that by hazard while looking for the correct validation method to implement in an EO of mine ;-) The _validateValueForKey() is used in Wonder for partial entities (which calls standard validateValueForKey() on each related partial). I think that feature is probably used only by very few so nobody noticed that coercing values in that method didn't make it into the DB.

@paulhoadley
Copy link
Contributor

@paulhoadley paulhoadley commented on 6665a30 Jun 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that feature is probably used only by very few so nobody noticed that coercing values in that method didn't make it into the DB.

Heh, I actually use partial entities all the time. Evidently I never ran into this. Good catch!

@paulhoadley
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By an amazing coincidence, I've just run into a failing unit test due to this change. Unless I'm abusing NSValidation in some way, this patch seems to change its behaviour. I've got a method validateClientRef() on Job that trims the Job.clientRef attribute and returns it—no one wants extraneous whitespace in the database. Here's the test:

@Test
public void duplicateJobIdsAfterTrimmingShouldNotBeAllowed() {
    job.setClientRef(CLIENT_REF + "  ");
    // Calling this first just causes NSValidation to take place.
    confirm(job, cannotBeSaved());
    // And here we confirm the input has been trimmed.
    assertEquals(CLIENT_REF, job.clientRef());
    return;
}

That test used to pass: the clientRef attribute was trimmed after validation took place. Now it fails.

I haven't stepped through ERXGenericRecord.validateValueForKey() in detail yet, but what do you think? Was I relying on a bug, or is NSValidation supposed to change the attribute value in this situation?

@paulhoadley
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the issue that you're calling _validateValueForKey() with value:

result = _validateValueForKey(value, key);

and as noted, the default implementation there just returns value. So by the end of the try block, result always equals value (unless _validateValueForKey() has been overridden). I'm not sure what validateObjectWithUserInfo() does, so I don't have anything to say about that if block, but I suspect the final line of the try should be:

result = _validateValueForKey(result, key);

Thoughts?

@darkv
Copy link
Member Author

@darkv darkv commented on 6665a30 Jul 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably you are correct. Though this means that if you are using partials the validate methods of the partials will get potentially an already coerced value and not the original one if you modify it in the "main" validate method. But I see that it makes more sense to pass result instead of original value to stack validation.

But if we change this behavior then we have to think about the method ERXPartialGenericRecord._validateValueForKey too:

protected Object _validateValueForKey(Object value, String key) throws ValidationException {
    Object result = value;
    for (ERXPartial partial : _partials()) {
        result = partial.validateValueForKey(value, key);
    }
    return result;
}

Here we probably should pass result too?

@paulhoadley
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though this means that if you are using partials the validate methods of the partials will get potentially an already coerced value and not the original one if you modify it in the "main" validate method.

Yes I see what you mean, but I think the convention in partials would be to only implement validation methods for keys you add to the base class. I think in reality there wouldn't be any "stacking", we'd just see super.validateValueForKey(value, key) validate the base class's keys, and _validateValueForKey(result, key) the partial's keys. What's this bit doing?

EOClassDescription cd = classDescription();
if (cd instanceof ERXEntityClassDescription) {
    ((ERXEntityClassDescription) cd).validateObjectWithUserInfo(this, value, "validateForKey." + key, key);
}

Here we probably should pass result too?

Interesting. I think, by convention, partial entities should only validate keys they add, so that in reality partial.validateValueForKey(value, key) would only modify value once in that loop, and it wouldn't make a difference. But maybe we should make it more explicit what's going on in both places, and probably add this convention to the partials documentation.

@paulhoadley
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe take this up on the mailing list for wider discussion?

@darkv
Copy link
Member Author

@darkv darkv commented on 6665a30 Jul 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure. I think passing result as param on line 1128 as you noted should be made in any case. I will push a commit in a couple of minutes. How to proceed with the _validate method for partials could be discussed on the mailing list though the logical answer would be to make this on par.

@paulhoadley
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, my attempt to open this up to the list didn't really take off! I think we have two outstanding problems:

  • I'm still not sure we're doing the right thing with validateObjectWithUserInfo() in this method, because I don't know what that's supposed to do.
  • We should make the corresponding change to ERXPartialGenericRecord._validateValueForKey(). Or at least document what's going on there—what do you think? (That is, in practice it shouldn't make any difference, but it looks like it might.)

@darkv
Copy link
Member Author

@darkv darkv commented on 6665a30 Aug 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure we're doing the right thing with validateObjectWithUserInfo() in this method, because I don't know what that's supposed to do.

Looking at it it seems to be a way to make validation by putting your validation rules into Entity Modeler instead of coding them in Java into your EO classes. Considering that the value to validate passed to that method is value and not result is probably safe because you certainly won't use both approaches concurrently but I think logically it is incorrect. It should be better documented in the Javadocs so it is clear that first the EO validation methods will possibly alter the value before then the userInfo related validation is applied.

We should make the corresponding change to ERXPartialGenericRecord._validateValueForKey(). Or at least document what's going on there […]

Yes to both. I will prepare a pull request for that.

@darkv
Copy link
Member Author

@darkv darkv commented on 6665a30 Aug 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah wait, I overlooked that ERXEntityClassDescription.validateObjectWithUserInfo() does not have a return value. This points to that it is only meant to validate the value but not to coerce the value (like making a string always uppercase, …). I would still argue that it makes more sense to do that on result instead of value though.

@paulhoadley
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at it it seems to be a way to make validation by putting your validation rules into Entity Modeler instead of coding them in Java into your EO classes.

I hadn't noticed it until just now, but yes—this is actually documented in the class-level Javadocs for ERXEntityClassDescription. I agree that it's inconsistent. Do you want to change the argument there to result as well?

The problem here is that each of these validation approaches needs to be given the chance to run (and the value not discarded), but there really needs to be an agreement by convention that you'd only ever use one of these approaches at most for any given key.

@paulhoadley
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah wait, I overlooked that ERXEntityClassDescription.validateObjectWithUserInfo() does not have a return value.

Oh right—that was lost on me as well. Good catch.

I would still argue that it makes more sense to do that on result instead of value though.

Agree. Are you happy to make the change?

@darkv
Copy link
Member Author

@darkv darkv commented on 6665a30 Aug 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check #791 if it seems ok.

Please sign in to comment.