[BUG] jsonget fails on inherited properties

andrewgoz commented 1 week ago

Describe the bug

The change (which would first appear in 5.3.2):

https://github.com/Jermolene/TiddlyWiki5/commit/ab72cc7b097a504fc2ab6edadfb0e987075f1475

added a Object.hasOwnProperty() check to getDataItem() via getItemAtIndex() and $tw.utils.hop().

When using the filter operator jsonget on JavaScript objects, inherited non-Array properties now return undefined instead of the property value.

Expected behavior

To generate the labels, the chart plugin is using a filter "[<context>jsonget[chart],[data],[datasets],<di>,[label]]" with makeFakeWidgetWithVariables() and suitable variables. The problem is the "data" property of the "chart" object must be inherited, or at least fails the Object.hasOwnProperty() check. I expect to be able to access that property, even if it is inherited.

To Reproduce

The following is my wiki upgraded to 5.3.3:

https://www.scss.com.au/family/andrew/tiddlywiki/tw-5.3.3.html#ChartJS%20Timeline%20Test

The labels show incorrectly as "0", "1", "2".

The following is my wiki, which is 5.3.0 as I write this issue report:

https://www.scss.com.au/family/andrew/tiddlywiki/#ChartJS%20Timeline%20Test

The labels show correctly as "V1", "V2", "V3".

Screenshots

No response

TiddlyWiki Configuration

Version 5.3.3
Saving mechanism Timimi
Plugins installed many, but specifically my ChartJS plugin

Desktop (please complete the following information):

OS: Windows
Browser Chrome
Version 125

Additional context

The issue can be fixed by changing the getItemAtIndex() function in core/modules/filters/json-ops.js to the following:

function getItemAtIndex(item,index) {
    if(!$tw.utils.hop(item,index) && $tw.utils.isArray(item)) {
        index = $tw.utils.parseInt(index);
        if(index < 0) { index = index + item.length };
    }
    return item[index];
}

pmario commented 1 week ago

It's hard to verify your changes and testing your example is close to impossible, because we would need to reverse engineer it. TW has a test edition. It would be nice, if you could create a new test, that fails with v5.3.4-pre with the existing code.

eg:

Create a file: test-json-filters-hop.tid with the following content
change the text of the tiddler title: "First" to the exact structure, that your "failing" JSON has
Then change the test it("should test .hop with jsonget", function() { to your filter

/*\
title: test-json-filters-hop.js
type: application/javascript
tags: [[$:/tags/test-spec]]

Tests the JSON filters and the format:json operator

\*/
(function(){

/* jslint node: true, browser: true */
/* eslint-env node, browser, jasmine */
/* eslint no-mixed-spaces-and-tabs: ["error", "smart-tabs"]*/
/* global $tw, require */
"use strict";

describe("json filter tests - hop and jsonget", function() {

    var wiki = new $tw.Wiki();
    var tiddlers = [{
        title: "First",
        text: '{"a":"one","b":"","c":1.618,"d": {"e": "four","f": ["five","six",true,false,null]}}',
        type: "application/json"
    }];

    wiki.addTiddlers(tiddlers);

    it("should test .hop with jsonget", function() {
//      expect(wiki.filterTiddlers("[{First}jsonget[b]]")).toEqual([]);
        expect(wiki.filterTiddlers(/* Your filter comes here */)).toEqual(/* what is the expected return json */);
    });

});

})();

Hope that makes sense.

andrewgoz commented 1 week ago

Thanks. The problem is that I'm not dealing with JSON, rather a JavaScript object hierarchy. A pure JSON data structure parsed into a JS object will never have properties that return false to Object.hasOwnProperty().

Am I able to create a variable, for example called "problem_obj", such that I can do something like:

expect(wiki.filterTiddlers("[<problem_obj>jsonget[blah_blah_blah]]")).toEqual(/*expected result here*/)

?

I don't know how to test this:

/*\
title: test-json-filters-hop.js
type: application/javascript
tags: [[$:/tags/test-spec]]

Tests the JSON filters and the format:json operator

\*/
(function(){

/* jslint node: true, browser: true */
/* eslint-env node, browser, jasmine */
/* eslint no-mixed-spaces-and-tabs: ["error", "smart-tabs"]*/
/* global $tw, require */
"use strict";

describe("json filter tests - hop and jsonget", function() {

    var wiki = new $tw.Wiki();
    class problem_obj {
        constructor() { this.f = 'data'; }
        hasOwnProperty(p) { return false; }
    }
    var widget = $tw.rootWidget.makeFakeWidgetWithVariables({"problem_obj":new problem_obj()});

    it("should test .hop with jsonget", function() {
        expect(wiki.filterTiddlers("[<problem_obj>jsonget[f]]",widget)).toEqual("data");
    });

});

})();

Jermolene commented 1 week ago

Thanks @andrewgoz. I think the bug here is that makeFakeWidgetWithVariables should not be allowing variables to be specified with values that are not strings. The design of the system does not properly support other types of variable. So, the workaround would be for your code to JSON.Stringify() the data, perhaps with a replacer function to handle the non-JSON properties.

andrewgoz commented 1 week ago

Sadly, JSON.Stringify() can't be used because the objects I'm dealing with are third-party charting library configuration objects that I have no control over and contain circular references (at least, the very first one I checked did). Trying to handle that so that JSON.Stringify() works doesn't seem practical.

I have to say, that since the jsonget operator is notionally only supposed to be used with JSON objects, that the check for hasOwnProperty would seem to be redundant and unnecessary since it would always be true? Maybe the function in question is used in other circumstances...

I've been able to change my chart usage so this issue isn't a problem any more, so my plugins and wikis are good for 5.3.4. If makeFakeWidgetsWithVariables is likely to become more strict in the future, then I have to re-think how my plugins are doing things.

pmario commented 1 week ago

@andrewgoz wrote:

I don't know how to test this:

Sorry, I have not been specific enough. But it seems your "test" was good enough for Jeremy to see, what's going on. That's all what I wanted with my request. Getting a "minimal" test-case.

For the sake of completeness:

You need a TW development environment to run the tests.

TW has a test-edtion, with several test-tiddlers: https://github.com/Jermolene/TiddlyWiki5/tree/master/editions/test/tiddlers/tests

For my example I did use: test-json-filters.js as a template and stripped it down to the bare minimum.

So if you would save your test code to the /test/ directory and then run tiddlywiki from the command line, all the tests will be checked.

cd /your/path/to/TiddlyWiki5
tiddlywiki ./edtions/test --build index

andrewgoz commented 6 days ago

Thanks guys. I'm going to close this off. I've updated my code to stop abusing jsonget. The new way of doing things even looks a bit nicer.

Jermolene / TiddlyWiki5