There's a small logic bug in GroupRoleStorage::loadByUserAndGroup() that can lead you to weird results if it is invoked more than once within the same request for a specific user/group combination. The issue is that results are cached locally, but that cache does not vary based on all the method arguments. (i.e. It doesn't account for the $include_implied argument.)

For example, let's say I have group roles A & B in a given group. I invoke loadByUserAndGroup($me, $my_group, TRUE) and I get back something like [A, B, member]. Later within the same request I only want to get those custom group roles, so I invoke loadByUserAndGroup($me, $my_group, FALSE).

Expected:
[A, B]

Actual:
[A, B, member] (the cached result from the previous call)

CommentFileSizeAuthor
#12 group-2896603-12.patch11.61 KBkristiaanvandeneynde
#12 interdiff-11-12.txt1.02 KBkristiaanvandeneynde
#11 interdiff-9-11.txt1.21 KBkristiaanvandeneynde
#11 group-2896603-11.patch11.59 KBkristiaanvandeneynde
#9 group-2896603-9.patch11.58 KBkristiaanvandeneynde
#6 static-caching-bug-2896603-6.patch5.32 KBmikran
#6 interdiff.txt2.62 KBmikran
#3 interdiff.txt925 byteskevin.dutra
#3 static-caching-bug-2896603-3.patch4.33 KBkevin.dutra
#2 static-caching-bug-2896603-test-only.patch3.33 KBkevin.dutra

Comments

kevin.dutra created an issue. See original summary.

kevin.dutra’s picture

kevin.dutra commented
Status: Active » Needs review
StatusFileSize
new static-caching-bug-2896603-test-only.patch3.33 KB

Here's a test that highlights the issue.

kevin.dutra’s picture

kevin.dutra commented
StatusFileSize
new static-caching-bug-2896603-3.patch4.33 KB
new interdiff.txt925 bytes

And now with the actual fix.

kevin.dutra’s picture

kevin.dutra commented
Assigned: kevin.dutra » Unassigned
danmuzyka’s picture

danmuzyka commented
Status: Needs review » Reviewed & tested by the community

This looks really good, and the test seems to cover the various combinations of conditions one would expect. Thank you for documenting each scenario in the test so clearly with code comments as well!

mikran’s picture

mikran commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new interdiff.txt2.62 KB
new static-caching-bug-2896603-6.patch5.32 KB

There is also a line

@todo Perhaps we need to be able to clear this cache during runtime?

that I stumbled upon while writing access tests for my custom code. This is very closely related to this patch in here and the test is easy to extend for this purpose too so I extended the patch a bit. Sorry that I have to put the issue back to needs review though.

dravenk’s picture

dravenk
He/Him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Static Caching, +user roles, +user permissions
Related issues: +#2906082: Figure out a way to cache lists using group permissions

I was tested this patch. It's working perfect.And, It could be fixed another issues #2906082: Figure out a way to cache lists using group permissions .

skyredwang’s picture

skyredwang
Location Shanghai
commented
Status: Reviewed & tested by the community » Needs review

We need more review.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
StatusFileSize
new group-2896603-9.patch11.58 KB

Ugh crap, I have been working on this myself and only now notice there's an issue for it. The attached patch fixes the problem, introduces a method for clearing the cache and clears it at the right times. Obviously everyone here will receive credit.

Status: Needs review » Needs work

The last submitted patch, 9: group-2896603-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs work » Needs review
Issue tags: -Static Caching, -user roles, -user permissions
StatusFileSize
new group-2896603-11.patch11.59 KB
new interdiff-9-11.txt1.21 KB

My bad

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
StatusFileSize
new interdiff-11-12.txt1.02 KB
new group-2896603-12.patch11.61 KB

And again... lack of sleep :P

The last submitted patch, 11: group-2896603-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

lawxen’s picture

lawxen commented
Status: Needs review » Needs work

I use jsonapi to test the patch
jsonapi/group/business&include=field_test
If I use a wrong user/password, I will get

{
    "data": [],
    "meta": {
        "errors": [
            {
                "title": "Forbidden",
                "status": 403,
                "detail": "The current user is not allowed to GET the selected resource.",
                "links": {
                    "info": "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4"
                },
                "code": 0,
                "id": "/group--business/727dce72-35d3-472a-b4bd-ebff35f98aed",
                "source": {
                    "pointer": "/data"
                }
            }
        ]
    },
    "links": {
        "self": "http://spark-de.dn8/jsonapi/group/business?include=field_business_logo"
    }
}

Then If use right user/password, get the same result.

After clear the cache, I will get the right result,
So the patch didn't resolve the cache problem.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs work » Needs review

This patch contains zero database caching. So even if clearing the caches solves the problem in #14, it definitely does not seem to be related to the issue in this patch.

This patch is about a static cache, your problem seems to be about a database cache.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.