== Background
Most of this was introduced in commit 5c335f9d77 (I1eb897c2cea3f5b7).
The original motivation was:
* Ensure wrappers like MultiWriteBagOStuff naturally do the right
thing. In practice, makeKey() results are interchangeable, with
the most contrained one (Memcached) also generally used as the first
tier. However, this is not intuitive and may change in the future.
To make it more intuitive, the default implemention became known
as "generic", with proxyCall() responsible for decoding these,
and then re-encoding them with makeKey() from the respective
underlying BagOStuff. This meant that MultiWriteBag would no longer
use the result of the Memcached-formatted cache key and pass it
to SqlBagOStuff.
* Allow extraction of the key group from a given key cache,
for use in statistics.
Both motivations remains valid and addressed after this refactor.
== Change
* Remove boilerplate and indirection around makeKey from a dozen
classes. E.g. copy-paste stubs for makeKey, makeKeyInternal, and
convertGenericKey.
Instead, let BagOStuff::makeKey and ::makeKeyInternal hold the
defaults. I believe this makes the logic easier to find, understand,
and refer to.
The three non-default implementations (Memcached, WinCache, Sql)
now naturally reflect what they are in terms of business logic,
they are a method override.
Introduce a single boolean requireConvertGenericKey() to let the
three non-default implementations signal their need to convert
keys before use.
* Further improve internal consistently of BagOStuff::makeKeyInternal.
The logic of genericKeyFromComponents() was moved up into
BagOStuff::makeKeyInternal. As a result of caling this directly
from BagOStuff::makeKey(), this code now sees $keyspace and $components
as separate arguments. To keep the behaviour the same, we would
have to either unshift $keyspace into $components, or duplicate
the strtr() call to escape it.
Instead, excempt keyspace from escaping. This matches how the most
commonly used BagOStuff implementations (MemcachedBag, and SqlBag)
already worked for 10+ years, thus this does not introduce any new
responsibility on callers. In particular, keyspace (not key group)
is set by MediaWiki core in service wiring to the wiki ID, and so
is not the concern of individual callers anyway.
* Docs: Explain in proxyCall() why this indirection and complexity
exists. It lets wrapping classes decode and re-encode keys.
* Docs: Explain the cross-wiki and local-wiki semantics of makeKey
and makeKeyGlobal, and centralise this and other important docs
about this method in the place with the most eye balls where it is
most likely seen and discovered, namely BagOStuff::makeKey.
Remove partial docs from other places in favour of references to this one.
Previously, there was no particular reason to follow `@see IStoreKeyEncoder`
much less to know that it holds critical that communicate the
responsibility to limit the key group to 48 chars.
* Docs: Consistently refer to the first component as the "key group",
thus unifying what was known as "key class", "collection",
"key collection name", or "collection name".
The term "key group" seems to be what is used by developers in
conversations for this concept, matching WMF on-boarding docs and
WMF's Grafana dashboard for WANObjectCache.
Change-Id: I6b3167cac824d8bd8773bc66c386f41e4d380021
We'd lose more useful coverage and spend more effort keeping these
accurate through refactors etc than is worth the theoretically "bad"
accidental coverage. Plus, even then we still very often forget to
update these and waste time digging into seemingly uncovered code
and either write duplicate test or discover they are already covered.
Especially for private methods this can be a flawed endavour as
we tend to trust tests when changing those and thus feel we shouldn't
update tests, except it's riddled with private method mentions for
coverage purposes (not in terms of actual called code).
I think the best practice is to write good narrow unit tests,
not to write bad tests and then hide their coverage. Generally
that's what we do. Maybe these are useful when invoking a huge
legacy class, but generally we don't need these I think, at least
in the libs our team maintains.
Change-Id: I4c7d826c7ec654b9efa20778c46c498784661f1c
The new style of checking for the last error during a section of
calls is more robust since it allows nesting of callers. Typically,
an external caller will want to watch a section of code that will
involve zero or more internally watched sections. Errors that are
seen internally (leading to a failing response) should also be
visible externally.
Replace internal BagOStuff clearLastError() calls.
Replace WANObjectCache clearLastError() calls. Such a class should not
clear the error codes since the class is effectively "internal". Callers
that are more meaningfully "external" might want to check the errors.
Cleanup "last" error handling for proxy backends.
Change-Id: I281817a85602967c0ec2bdd23a5d8be101680b64
Ended up using
grep -Prl '\->setMethods\(' . | xargs sed -r -i 's/setMethods\(/onlyMethods\(/g'
special-casing setMethods( null ) -> onlyMethods( [] )
and then manual fix of failing test (from PS2 onwards).
Bug: T278010
Change-Id: I012dca7ae774bb430c1c44d50991ba0b633353f1
Add "generic" key methods for quickly deriving keys from
key component lists in a bijective manor. This is useful
for BagOStuff classes that wrap other BagOStuff instances
or for parsing keys to get stats.
Make the proxy BagOStuff classes (ReplicatedBagOStuff,
MultiWriteBagOStuff, CachedBagOStuff) use "generic" keys
so that they can convert to appropriate keys when making
backing cache instance method calls.
Make EmptyBagOStuff, HashBagOStuff, APCUBagOStuff,
RedisBagOStuff, and RESTBagOStuff use "generic" keys rather
than those of MediumSpecificBagOStuff::makeKeyInternal().
This lets proxy BagOStuff classes bypass key conversions
when used with instances of these classes as backing stores.
Also:
* Fix missing incr(), incrWithInit(), and decr() return
values in MultiWriteBagOStuff.
* Make MultiWriteBagOfStuff, ReplicatedBagOStuff, and
CachedBagOStuff use similar backend method forwarding
styles by using a new BagOStuff method.
* Improved various related bits of documentation.
Bug: T250239
Bug: T235705
Change-Id: I1eb897c2cea3f5b756dd1e3c457b7cbd817599f5
The name change happened some time ago, and I think its
about time to start using the name name!
(Done with a find and replace)
My personal motivation for doing this is that I have started
trying out vscode as an IDE for mediawiki development, and
right now it doesn't appear to handle php aliases very well
or at all.
Change-Id: I412235d91ae26e4c1c6a62e0dbb7e7cf3c5ed4a6
This should help fix the following issues that various repos
are getting from Phan as of late:
> Call with 5 arg(s) to \BagOStuff::makeKey() which only takes 2 arg(s)
> defined at ../../includes/libs/objectcache/BagOStuff.php:456
> <source="PhanParamTooMany"/>
Bug: T228563
Depends-On: I5cfba063821101325a5a7359e6b8ad71a0fb1b2f
Depends-On: Ifa5b96735376f2fbe3680799f960616ba8d357ff
Change-Id: Ic9df7f3ad7f356c7cbdfe1edfbe35821b931dda6
This reduces confidence in the test. There is no guruantee that
it won't return the same value twice during the duration of a full
PHPUnit run of all test suites, whether twice in a row or 20 minutes
apart.
For a test that needs a string of any kind, use an explicit, consinstent
and cheap literal value.
For a test that specifically needs some kind of uniqueness compared to
something else within the same test case, do so explicitly.
Tests that require something globally unique (for some undefined/vague
definition of "global") were not found, and should not exist anyway.
Also, in libs/objectcache tests, fix order of parameters in some
assertions (expected first, then actual), and use assertFalse/assertSame
instead of assertEqual for cases where false is expected to remove
tolerance of other loosely equal values.
Change-Id: Ifc60e88178da471330b94bfbf12e2731d2efc77d
If someone writes an object into a BagOStuff, they typically expect that
later changes to the object will not affect the value stored.
MultiWriteBagOStuff's async write handling was violating this
expectation, which is potentially causing T168040.
Bug: T168040
Change-Id: Ie897b900befdc8998614af06f9339cd07665703e
Similar to what WANObjectCache and CachedBagOStuff are already doing.
Also add missing tests for WANObjectCache (similar to those for CachedBagOStuff).
Bug: T167465
Change-Id: I1a0c9324726aa6a1b221def985773b1b819181fd
