Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extending the lib with possibility to generate flat tables #50

Closed
kirill533 opened this issue May 26, 2021 · 7 comments
Closed

Extending the lib with possibility to generate flat tables #50

kirill533 opened this issue May 26, 2021 · 7 comments

Comments

@kirill533
Copy link

I have added extra functionality in my fork. Unfortunatelly I could not extend the by including the lib as dependency and had to modify the core files in the fork.

I think, this could be a great extension and might become the part of the main lib.

Here is the usage example:

JSON:

{
   "store":{
      "name":"My Store",
      "book":[
         {
            "category":"reference",
            "author":"Nigel Rees",
            "title":"Sayings of the Century",
            "price":8.95,
            "available":true,
            "authors":[
               "Nigel Rees"
            ]
         },
         {
            "category":"fiction",
            "author":"Evelyn Waugh",
            "title":"Sword of Honour",
            "price":12.99,
            "available":false,
            "authors":[
               
            ]
         },
         {
            "category":"fiction",
            "author":"Herman Melville",
            "title":"Moby Dick",
            "isbn":"0-553-21311-3",
            "price":8.99,
            "available":true,
            "authors":[
               "Nigel Rees"
            ]
         },
         {
            "category":"fiction",
            "author":"J. R. R. Tolkien",
            "title":"The Lord of the Rings",
            "isbn":"0-395-19395-8",
            "price":22.99,
            "available":false,
            "authors":[
               "Evelyn Waugh",
               "J. R. R. Tolkien"
            ]
         }
      ],
      "bicycle":{
         "color":"red",
         "price":19.95,
         "available":true,
         "model":null,
         "sku-number":"BCCLE-0001-RD"
      },
      "bicycleSet":[
         {
            "color":"red",
            "price":19.95,
            "available":true,
            "model":null,
            "sku-number":"BCCLE-0001-RD"
         },
         {
            "color":"green",
            "price":19.75,
            "available":false,
            "model":null,
            "sku-number":"RCADF-0002-CQ"
         }
      ]
   },
   "authors":[
      "Nigel Rees",
      "Evelyn Waugh",
      "Herman Melville",
      "J. R. R. Tolkien"
   ],
   "Bike models":[
      1,
      2,
      3
   ]
}

The code:

        $jsonObject = new JsonObject($json);
        $result = $jsonObject->getTable('[$.store.book[*].price,$.store.book[*].price,$.store.book[*].authors[*],$.store.book[*].title,$.store.name]');

The result:

                array(
                    array(8.95, 8.95, "Nigel Rees", "Sayings of the Century", "My Store"),
                    array(12.99, 12.99, null, "Sword of Honour", "My Store"),
                    array(8.99, 8.99, "Nigel Rees", "Moby Dick", "My Store"),
                    array(22.99, 22.99, "Evelyn Waugh", "The Lord of the Rings", "My Store"),
                    array(22.99, 22.99, "J. R. R. Tolkien", "The Lord of the Rings", "My Store")
                ),

My fork with the implementation: https://github.com/kirill533/JsonPath-PHP

@Galbar
Copy link
Owner

Galbar commented May 26, 2021

Hi @kirill533!

I've looked at your solution. I was surprised that you could not do this already without modifying the code.

I think that, instead of adding the table-building logic, it'd be better to identify what's missing from the library that prevented you from implementing the table-building logic without having to mess with the internals.

Could you share what obstacles did you meet while trying to build it? For starters I can think of two:

  • Get the key of a value (to group the values by their $.store.book array index). Returning key of found object #46 could solve this
  • Some way of getting the index key and the result of a subpath. This one has me thinking. Maybe extend the child selector operator [] to accept, not only child names but also child paths (i.e. $.store.book[@~, @.price] would return [0, 8.95, 1, 12.99, 2, 8.99, ...]

What do you think? Did I miss anything other that also prevented you from implementing this?

@kirill533
Copy link
Author

Hi @Galbar , thanks for the fast reply.

I could find 2 reasons, why I could not do the implementation separatelly from the core logic of the lib:

For the efficiency and for simplicity of the solution I did not want the full path to be resolved for each "column" in the "table". Therefore I needed to call JsonObject::getReal method with different first argument (passing a subtree instead of $this->jsonObject).

The method JsonObject::getReal is private, so the only way was to rewrite the class.

For some Json structures with one to many relationships I need to link certain values between each other. (Example: two authoers of "The Lord of the Rings" are recorded as two records).

In order to be able to do it, I need to perform some extra parsing of jsonpath. I did parcing of the jsonpath into a tree structure where I can see how different values relate to each other. So, I am also using the private method \JsonPath\JsonObject::matchValidExpression as part of the jsonpath parcing logic.

@Galbar
Copy link
Owner

Galbar commented May 26, 2021

So, the function that you describe in the example above does this, right?

function getTable($jsonObject)
{
    $books = $jsonObject->getJsonObjects('$.store.book[*]');
    $storeName = $jsonObject->get('$.store.name')[0];
    $table = array();
    foreach ($books as $book) {
        list($price, $authors, $title) = $book->get('$[price, authors, title]');
        if (empty($authors)) {
            $table[] = array($price, $price, null, $title, $storeName);
        } else {
            foreach ($authors as $author) {
                $table[] = array($price, $price, $author, $title, $storeName);
            }
        }
    }
    return $table;
}

If this is correct, then the library already provides the tools for doing this for a specific case. This is something that I wanted to confirm.

Now, I imagine the challenge you are met with is implementing a generic solution, correct? And, as you say, the big obstacle was that the functions for parsing the jsonpath language are private.

I'm thinking it'd be nice to expose them somehow. I'm sure most, if not all, of them could be made stateless and put in a different file, maybe a subdirectory. JsonObject would be built from composing them and it would allow to implement extensions as the one you propose.

I like your idea but I don't think its place should be inside the JsonObject. You had to add it there because that was the only way to implement it. This shows a mistake in the library architecture. I mean, you basically implemented your own way of querying the data, using the existing fuctionality to parse the language, take some decisions and then query the array.

Let me think a bit more about it. Although I'm pretty sure the first step would be extracting the core parsing and querying logic from the JsonObject.

@kirill533
Copy link
Author

Hi @Galbar,

thank you for code sample. This is indeed interesting implementation that I did not consider. For my current needs I am ok with the current solution in my implementation, even though it may require extra work to maintain it. This is just a small part of the software I am building.

My goal was more to contribute to your library with extra functionality. And it is less about making my solution more maintanable.

I am completelly sharing your opinion and like the direction that you pointed out. What I did not understood is if you would like to include the functionality of getTable() to the jsonpath library, or if you see it more logical to keep it in separate library (that uses jsonpath as dependency after refactoring at some point in the future)?

@Galbar
Copy link
Owner

Galbar commented May 28, 2021

Thanks for the contribution, I really appreciate it. It made me realize of an use case that I hadn't thought of before: custom logic built on top of the language parser.

I'm not going to include getTable() in this library. I think its place is outside of this library, probably using this library as a depencency when the refactor is done.

I'm slowly working in extracting the private functionality to public methods that are part of the library.

I can't promise an ETA, because I have the time I have, but I'll use this issue to keep you posted.

@Galbar
Copy link
Owner

Galbar commented Jun 1, 2021

Hi @kirill533,

I've opened #52. I won't merge it yet. I want to add some docs and update the readme (this PR alone will increase a lot the API surface of the library 🤦).

I was wondering if you could give it a look and let me know if the exposed functionality would allow you to create your getTable()?

@Galbar
Copy link
Owner

Galbar commented Jul 26, 2021

I just merged the PR

@Galbar Galbar closed this as completed Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants