Open alxndrsn opened 5 years ago
@alxndrsn Thanks for posting, we'll take a look as soon as possible.
For help with questions about Sails, click here. If you’re interested in hiring @sailsbot and her minions in Austin, click here.
@alxndrsn Thanks for the detailed information for these behaviors. The expectations are definitely helpful to update the error responses. What are your thoughts on using the .where() clause to find()
without limit()
for a column called min
?
What are your thoughts on using the .where() clause to find() without limit() for a column called min?
I haven't tried this, but I guess it could be a workaround. In my use case .limit(2)
is sufficient. It may also be more correct, as I only actually want the first result, but if there is more than one result then a warning is logged.
@alxndrsn this error is there because in older versions of Waterline, the aggregation clauses sum
, average
, min
, max
, and groupBy
were supported in criteria.
Because those used to be reserved for criteria modifiers (and wouldn't have been usable as attribute names in the past), Waterline triggers an error for the benefit of people who have upgraded from an older version, to help with updating their existing applications.
So at least for the moment, Waterline doesn't support queries that include those attribute names in the criteria, but this behavior may change in future versions once more people have been on v0.13 long enough for those old criteria modifiers to have been forgotten (this would be a good candidate for Sails v2, since by then people will have gone through two upgrade guides). Wish I could help you out more, but in the mean time I would recommend renaming that attribute to minimum
if possible to avoid conflicts.
Thanks for the background.
Wish I could help you out more, but in the mean time I would recommend renaming that attribute to
minimum
if possible to avoid conflicts.
If you don't recommend using min
as a column name, perhaps it could be officially documented as reserved (as previously), and disallowed up-front?
Otherwise, I think the error mesage could still be improved to avoid confusing users who are not upgrading from previous versions. Current error message:
In previous versions,
min
could be provided in a criteria to perform an aggregation query. But as of Sails v1.0/Waterline v0.13, the usage has changed. Now, to calculate the minimum value of an attribute across multiple records, use the.find()
model method.
Maybe adding something like this would help:
Alternatively, if you are using
min
as a column/attribute name then please be advised that some things won't work as expected.
If you don't recommend using min as a column name, perhaps it could be officially documented as reserved (as previously), and disallowed up-front?
@alxndrsn I think we could do that, although we're hoping for future versions of waterline will phase out the min
/max
stuff entirely (in favor of the simple, top-level aggregation methods like .sum()
).
So I think your other idea might make more sense. And I think your suggested wording is excellent. If you find a moment to submit a PR to add your language here and drop a link here so we're sure to see it, I'd like to review and merge it.
Thank you!
Side note, just so you know for future reference: While I don't think we should do this here, as far as load-time checks, ideally, the best place for that would be in waterline core itself. But that's just not how things are actually structured at the moment. So the best place for them right now is in sails-hook-orm (because in some cases we use the configured sails logger for warnings). We also do some validation in waterline-schema for historical reasons, but that's something I'm hoping to phase out over time so that we can consolidate all load-time checks in one place. (We mainly overhauled post-load error handling/validation in 2016-2017 because that was a higher priority and the absent of stricter usage checks were confusing a lot of people - but the load-time checks still leave much to be desired, tbh. Something to work towards bit by bit!)
@alxndrsn thanks! merged (with a couple additions - see https://github.com/balderdashy/waterline/pull/1600#issuecomment-488094564)
Sails version: 1.1 Node version: v10.15.1 NPM version: 6.4.1 DB adapter name: N/A DB adapter version: N/A Operating system: OSX
Observed
Doing
find()
calls on a db when matching a column namedmin
fails withoutlimit()
clause.Recreating
Example project at: https://github.com/alxndrsn/sailsjs-min-without-limit
With
limit()
:Without
limit()
:Expected
min
as a column name completely, orfind()
withoutlimit()
for a column calledmin