The performance decreased because of a combination of calls that were supposed to increase performance.
We have a mechanism in place which automatically eager loads models needed in the element representers when a collection of them is rendered. This is to avoid N+1 queries of course. But, if eager loading is combined with e.g., a LIMIT, which we do because we paginate, rails automatically falls back to issuing two instead of just one SQL statement. Which makes sense as otherwise LEFT JOINS might mess with the result set.
But Rails does so in a somewhat simple fashion. It uses the first query to get the DISTINCT ids. The second is used to load the values (without a limit). But instead of removing all WHERE statements in the second SQL statement and then apply just the one for the ids, it keeps the original WHERE statement and applies the one for the ids on top. The problem with that is that the database trips on that (I didn't check the why) and uses a less than optimal query plan.
That was the problem here as well. The first query remained reasonable quick (300ms) but the second one took 25s.
The fix is to split the two statements by hand in the representer whenever eager loading is defined. The first query has all the filters but no eager loading and fetches the ids. The second takes the ids, and with eager loading included loads the data. Et voila, second query takes 10ms.
That at least works for relations, work_packages and projects. But there are other representers that also seem to have custom behaviour. I'm looking into whether they can be easily adapted.
This error is intended for cases when a method is
intentionally not implemented, because the module/class defining
it expects a subclass (or class including the module) to implement
the method.
This is intended to distinguish it from other cases, such as:
* feature not implemented yet
* edge case of a method call not yet supported
Notably it avoids the misuse of the Ruby-defined NotImplementedError,
which is only intended for much more specific scenarios:
> Raised when a feature is not implemented on the current platform. For example, methods depending on the fsync or fork system calls may raise this exception [...]
Also see https://docs.ruby-lang.org/en/master/NotImplementedError.html
They both use `work_package` and `work_package_id` to render links,
which generates verbose deprecation warnings. They are filling up logs.
Force usage of `entity` and `entity_id` instead in the api representers
to reduce the burden.
Index endpoints will automatically discover and use
the `to_eager_load` and `to_preload` declarations of their
representers. This takes away the possibility of forgetting
to add those to the scope.
I found a few endpoint that didn't use them, when other endpoints
rendering the same collection did.
Interestingly the SCM integration modules consistently declared
those methods but never used them.
We were previously eager-loading work packages, but
when time entries were refactored to have a polymorphic association
to "entities", eager loading was not possible anymore and the eagerload
was removed without replacement (see eb4a1d7138).
However, preloading is a viable alternative for polymorphic associations,
so we use that now instead. Docs around the "to_eager_load" and the newly
added "to_preload" method have been added to clarify the purpose of these two
methods.
Adding this method to the result makes it available to any
component involved in the SqlWalker implementation, so it
should be more reusable.
The code that's calculating the actual SELECT SQL statements is
not making use of the new helper, so some duplication still exists,
but hopefully we'll not deduplicate more than the current state.
It's easily possible that the selects do not
require the calculation of the total number of elements.
However, since we resolve the number of total elements ahead
of constructing the complex SQL query, there's no way for the
database to optimize this out. Thus we have to manually skip
the total calculation if we do not need to perform it.
We will be rendering the elements on the current page anyways,
so it's unavoidable that we load them. By forcing us through to_a.size,
we make sure that the elements are loaded first and then only count how
many elements have been loaded. Since AR caches the result, the relation
will not be loaded twice.
Previously we had a useless SQL count query, which can be tremendously
slower than the actual loading of elements. In the case of time entries
for a larger database, the COUNT took ~1.5 seconds, when the loading
of those same elements only took ~20ms. I don't hope that this ratio
is generalizable to all collection API endpoints, but it's definitely
a nice speed boost.
Previously it was always necessary to provide a full list of users when calling the service.
Now it's also possible to provide a delta that should be achieved.
This allows to simplify calls to the service, when only the delta is known (e.g. "add this user").
It also makes those calls safer, since the internals of the Groups::UpdateService are already locked
through an advisory lock (via BaseContracted), thus concurrent changes to group memberships are serialized
properly inside the service. However, previous implementations would have read the current members of a group
outside of the service scope, where race conditions could have occured.
Using the config option, the project representer can hide all custom field values (link, property, embedded)
if the user does not have the necessary permission (:view_project)
Days are easier to work with than hours when building SQL requests.
Also change "everyday" value for overdue from 0 to 1, for consistency
with the other values.
will_paginated, when actually fetching the records will do an SQL count on top of the fetching. We don`t need that
in our offset paginated represters since those implement their own counts.
And if the per_page is 0, we don`t even have to try to fetch any objects so we can use `none` to avoid the database
roundtrip.
Oliver Günther