Adding a key with type Z8 to Z8 lead to a HTTP 500
Closed, ResolvedPublicBUG REPORT
Actions

Assigned To

Authored By

	DVrandecic
	Feb 2 2021, 1:03 AM

Description

The version of Z8 that looked fine was this one:

https://notwikilambda.toolforge.org/w/index.php?title=ZObject:Z8&action=raw&oldid=80

It had already the keys Z8K1-Z8K4.

I then added the Z8K5 key (I am pretty sure that's what I did):

That should look like this:

{
    "Z1K1": "Z3",
    "Z3K1": "Z8",
    "Z3K2": "Z8K5",
    "Z3K3": {
        "Z1K1": "Z12",
        "Z12K1": [
            {
                "Z1K1": "Z11",
                "Z11K1": "en",
                "Z11K2": "identity"
            }
        ]
    }
}

Once I did that, I get 500 Internal Server Error if I go to Z8, or action=raw, or history and diff any version, etc. E.g. https://notwikilambda.toolforge.org/wiki/ZObject:Z8

Here's a full copy of Z8 before I added the key above:

{

"Z1K1": "Z2",
"Z2K1": "Z8",
"Z2K2": {
    "Z1K1": "Z4",
    "Z4K1": "Z8",
    "Z4K2": [
        {
            "Z1K1": "Z3",
            "Z3K1": "Z10",
            "Z3K2": "Z8K1",
            "Z3K3": {
                "Z1K1": "Z12",
                "Z12K1": [
                    {
                        "Z1K1": "Z11",
                        "Z11K1": "en",
                        "Z11K2": "arguments"
                    }
                ]
            }
        },
        {
            "Z1K1": "Z3",
            "Z3K1": "Z4",
            "Z3K2": "Z8K2",
            "Z3K3": {
                "Z1K1": "Z12",
                "Z12K1": [
                    {
                        "Z1K1": "Z11",
                        "Z11K1": "en",
                        "Z11K2": "return type"
                    }
                ]
            }
        },
        {
            "Z1K1": "Z3",
            "Z3K1": "Z10",
            "Z3K2": "Z8K3",
            "Z3K3": {
                "Z1K1": "Z12",
                "Z12K1": [
                    {
                        "Z1K1": "Z11",
                        "Z11K1": "en",
                        "Z11K2": "testers"
                    }
                ]
            }
        },
        {
            "Z1K1": "Z3",
            "Z3K1": "Z10",
            "Z3K2": "Z8K4",
            "Z3K3": {
                "Z1K1": "Z12",
                "Z12K1": [
                    {
                        "Z1K1": "Z11",
                        "Z11K1": "en",
                        "Z11K2": "implementations"
                    }
                ]
            }
        }
    ],
    "Z4K3": "Z30"
},
"Z2K3": {
    "Z1K1": "Z12",
    "Z12K1": [
        {
            "Z1K1": "Z11",
            "Z11K1": "en",
            "Z11K2": "Function"
        }
    ]
}

}

Steps to Reproduce:

Go to https://notwikilambda.toolforge.org/wiki/ZObject:Z8

Actual Results:

500 Internal Server Error

Expected Results:

Display Z8

Details

	Subject	Repo	Branch	Lines +/-
	Add validation context to avoid infinite recursion	mediawiki/extensions/WikiLambda	master	+135 -1
	Remove check with type registry when validating a type reference	mediawiki/extensions/WikiLambda	master	+4 -1

Customize query in gerrit

Event Timeline

DVrandecic created this task.Feb 2 2021, 1:03 AM

DVrandecic moved this task from Phase β to To triage on the Abstract Wikipedia team board.

DVrandecic edited projects, added Abstract Wikipedia team; removed Abstract Wikipedia team (Phase β).

Yeah, I think it is the recursive usage of the key type. I tried the following Type definition here and it also leads to a 500.

Current ZObject: { "Z1K1": "Z2", "Z2K1": "Z44", "Z2K2": { "Z1K1": "Z4", "Z4K1": "Z44", "Z4K2": [ { "Z1K1": "Z3", "Z3K1": "Z44", "Z3K2": "Z44K1", "Z3K3": { "Z1K1": "Z12", "Z12K1": [ { "Z1K1": "Z11", "Z11K1": "en", "Z11K2": "identity" } ] } } ], "Z4K3": "Z30" }, "Z2K3": { "Z1K1": "Z12", "Z12K1": [ { "Z1K1": "Z11", "Z11K1": "en", "Z11K2": "Boolean" } ] } }

https://notwikilambda.toolforge.org/wiki/ZObject:Z44

DVrandecic moved this task from To triage to Phase γ on the Abstract Wikipedia team board.Feb 3 2021, 5:44 PM

DVrandecic edited projects, added Abstract Wikipedia team (Phase γ); removed Abstract Wikipedia team.

DVrandecic triaged this task as Medium priority.Feb 3 2021, 5:52 PM

This is interesting - I'll take a look at it!

Note that Z6K1 has value type Z6 and that doesn't cause this problem.

DVrandecic raised the priority of this task from Medium to High.Feb 10 2021, 5:28 PM

Ok, I've tracked down the source of the problem... It is indeed an infinite recursion issue, in the validation process.

In ZObjectFactory::validateKeyValue the 'Z8K5' key which itself has the 'Z3K1' type reference to 'Z8', hits this bit of code:

case ZTypeRegistry::HACK_REFERENCE_TYPE:
        if (
                is_string( $value )
                && ZKey::isValidZObjectReference( $value )
                && $registry->isZObjectKeyKnown( $value )

but unfortunately Z8 hasn't yet been registered in the type registry, so it goes off to fetch the Z8 object from the database again, which goes through validation yet again.

I can think of several ways to fix this, but probably we should think a bit about it:
(1) Register the type immediately, before trying to create it (so isZObjectKeyKnown() returns true the second time it is called on a type)
(2) Skip validation for persistent objects when fetching them (presumably they were validated when saved).
(3) Don't bother to check the type registry when validating a type reference (would allow creation of type references to types not yet created, this might be useful)
(4) Have a separate cache of Persistent ZObjects that allow skipping creation and checks etc. the second time a reference to one comes up.

or maybe some combination of these. @Jdforrester-WMF any thoughts on this? Anybody else?

Change 663685 had a related patch set uploaded (by ArthurPSmith; owner: ArthurPSmith):
[mediawiki/extensions/WikiLambda@master] Remove check with type registry when validating a type reference

https://gerrit.wikimedia.org/r/663685

gerritbot added a project: Patch-For-Review.Feb 11 2021, 9:41 PM

The above patch removes the type registry check (option 3). But maybe another solution would be better in the long run?

After discussion James suggested keeping track of the validation context (basically I think option 4 above) - I'll look into this.

Change 663685 abandoned by ArthurPSmith:
[mediawiki/extensions/WikiLambda@master] Remove check with type registry when validating a type reference

Reason:
after discussion we'll take a different approach here

https://gerrit.wikimedia.org/r/663685

Change 666648 had a related patch set uploaded (by ArthurPSmith; owner: ArthurPSmith):
[mediawiki/extensions/WikiLambda@master] Add validation context to avoid infinite recursion Note this currently only tracks validations for type objects; if there are other cases where an object might be repeatedly validated they could also be added as calls to the validatingInContext method.

https://gerrit.wikimedia.org/r/666648

Change 666648 merged by jenkins-bot:
[mediawiki/extensions/WikiLambda@master] Add validation context to avoid infinite recursion

https://gerrit.wikimedia.org/r/666648

Thank you, Arthur!

Adding a key with type Z8 to Z8 lead to a HTTP 500Closed, ResolvedPublicBUG REPORTActions

Description

Details

Event Timeline

Adding a key with type Z8 to Z8 lead to a HTTP 500
Closed, ResolvedPublicBUG REPORT
Actions