criteria field name misspelling -> strange behavior

[jpr]

Hello,
In a piece of my code, I update a row in a SQL server table. I'm doing this with the use of a criteria on PK column "status_id=12345". That works fine with dsRequest.sqlExecute(...,"update"...), and it's really easy.
But... this morning, I did a misspelling in the criteria fieldName: "statttttus_id" (instead of "status_id" in my ds file).
Such error should produce an dsRequest.Status error. Or maybe an exception?
Well instead of this, a wrong query was executed: "Update .... where (1=1)" -> all my table was update!!!
Can you please check about this?
Best regards
Jean-Philippe

[Isomorphic]

Bad criteria sent from the client produce a warning and are ignored. Updates are not allowed without a primary key unless you set allowMultiUpdate:true. If you have set allowMultiUpdate, you are responsible for making sure the criteria makes sense.

[jpr]

Hi,
thanks for this beautiful answer. But it would have been more pretty with:
"Sorry for this"
or... "Thanks for the suggestion, I will move it to the todo list"
or... "The code will be corrected to throw an exception if criteria fieldName is not found in dataSource fields"
or... "The framework will soon catch the responsibility to not do wrong queries like this one"
or... "Please check every fieldNames with datasource.getFields.getNames.contains() before execute, as our framework is not doing it"

Best Regards
Jean-Philippe

[Isomorphic]

Again, if you set allowMultiUpdate, you are responsible for making sure the criteria is correct. This is by design; there is no good way for the framework to, in general, verify that your criteria make sense, so we don't attempt any verification, and certainly we would not want to make it the case that *in this specific mode* extra data in the criteria causes an exception, when it is ignored for all other modes.

This will not be changed, so all of your alternative responses would have been inaccurate.

[jpr]

Hi,
"criteria that make sense"?
I perfectly care about providing a criteria with values that makes sense, and I can understand that it is not possible for the framework to know what are the rows to update.
But the framework is responsible to not execute criteria in a nonsense way,. I mean changing "......where stattttus_id=12345" to "......where (1=1)"
Please make your code less "wrongly" intelligent ;). The query should simply be "......where =12345" which will result in a real exception
Best regards
Jean-Philippe

[Blama]

Hi Isomorphic,

I agree with the OP. I also have a bad feeling when doing deletes/updates, or even selects where the result is used later on as list to delete or stuff like that - it's not limited to allowMultiUpdate.
Also displaying too much data from a DMI where the criteria generation has a bug might be a error source.
Therefore I'd also prefer if there was a mode where an exception is thrown instead of generating a WHERE (1=1).
This won't break existing applications or only ones, where an unnoticed bug exists. You could also let the setting default to false.

Perhaps something similar to Strict Mode?

Best regards
Blama

[claudiobosticco]

I also agree with Blama

[Isomorphic]

What you may be missing is that the system routinely ignores "extra" data in criteria, and your applications most likely rely on this. If we treated criteria strictly across the board, it would turn trivial coding errors into a failed fetch, and be a recurring source of hassle.

So what situations are you talking about for a "strict mode"?

All possible ops? Bad idea because of above.

Just updates with allowMultiUpdate? Again, we can't meaningfully verify your criteria, so you need code here making sure the criteria are correct anyway, so a strict mode seems superfluous.

Something different from any of these options?

[Blama]

Hi Isomorphic,

I can think of a few customOperation I'm using right now, that rely on this. Perhaps you are right then and a "full strict mode" would even make testing a hassle.
Generally the idea is to reduce a potential source of errors - mostly typos, but also system changes, where the developer did not do the change in every part of the application.

How about this:

• A new server setting "strictFieldNameChecks" in server.properties, defaults to false. If true, all requests (client and sever) will be checked in SQLDataSource(?), unless they include a disableStrictFieldNameChecks=true setting
• A new server-side property DSRequest.disableStrictFieldNameChecks in order to disable it on a per-request basis
• A new client-side property DSRequest.disableStrictFieldNameChecks in order to disable it on a per-request basis

Now, the default execution for all existing users won't change.
Users that want to can enable the strict checking and test their application. If ever a situation comes up where the server code throws an exception and I know it should not, I can set disableStrictFieldNameChecks=true for that Request.
More importantly I'll catch all cases where I rely on a non-existing DSField or simply have a typo.

Best regards
Blama

[Isomorphic]

This proposal doesn't make much sense to us - no one would turn that flag on since it would just create the need to turn it off over and over again, and still wouldn't reduce the effort of validating criteria in the one case it matters (allowMultiUpdate).

[jpr]

Hello isomorphic,

Well, it reminds me about VBA "option explicit" concept. I feel to be back to pre-historic age because of this missing functionnality :p

About ignoring "extra" data in criteria, that's non-sense., we have to trust the framework -> if a criteria is specified in the "where" clause, it has to be executed as specified (or to failed if wrong typo). Changing my query is a... framework "lie".
...

I can't understand you: I'm only proposing a way to improve your framework. So why you don't simply add this functionality to the list of "proposed improvements" (even if low priority)?

Best regards

Jp

[Isomorphic]

See above #8 - as we have already covered, this would not be an improvement.

[jpr]

In #8, I can see your question: "Something different from any of these options?"

Well, I'm really happy to help you here with another way: "to verify the criteria part only" for all possible ops. Because even a read can "functionally" result in a big mess with such framework misunderstanding around criteria parsing.

At least, you can still add a "documentation" improvement to warn about this problem. ;)

[Isomorphic]

We've explained above why this doesn't make sense (multiple times now). You seem to be just ignoring these explanations. Please don't keep posting here unless you have either a new suggestion, or are prepared to actually address the reasons the system is designed as it is.

[Isomorphic]

Let's just spell it out one more time for everyone:

For normal updates and removes, the system requires all PK fields to have values, and rejects the request if they do not. This includes submitting data with a typo in the PK field name - such a request will be rejected.

For allowMultiUpdate, the system allows blank criteria (affecting all rows), because that can be what's intended. It also makes no attempt to verify that the values in the criteria are correct in any way, because again the system has no way of knowing what you intend.

Therefore when using allowMultiUpdate, if you intend the criteria to be constrained in some way, you *must* write code to check it - there is no possible way the framework could do it for you. And if you write appropriate code to make sure expected criteria values are present, then your code will also catch a typo in a criteria field name.

Hence any feature to catch this for you is superfluous.