parkervcp / crocgodyl

golang pterodactyl terminal aplication and api interface
MIT License
10 stars 9 forks source link

Invalid unmarshall error on GetUsers #8

Closed ccuetoh closed 4 years ago

ccuetoh commented 4 years ago

The full error is: json: cannot unmarshal object into Go struct field Pagination.meta.pagination.links of type []crocgodyl.Links

The JSON provides a Links compatible object as the response, yet Croc will try to unmarshal it into the []Links slice.

PR #9 provides a fix

parkervcp commented 4 years ago

this is a much bigger issue than just GetUsers. It affects all the things I can find with pagination

ccuetoh commented 4 years ago

Well, yes and no. The problem is in the structure definition. It does not match the JSON object that the panel sends. That causes an unmarshal error every time that an object with Pagination is created.

The Go structure defines pagination.links as []Links type, it should be Link, and then Link is defined as an interface while it should be map[string]string.

Please see: https://github.com/CamiloHernandez/crocgodyl/commit/56d08124c5f14cbb91bc96bd1dcffa2274f12934

https://github.com/CamiloHernandez/crocgodyl/commit/73b2da436ad7708da5b2476f1bdd12589b7ffd96

parkervcp commented 4 years ago

The panel uses laravel and I found the exact issue. php json_encode replaces an empty object links:{} with an empty array links:[]

this is a broken approach to how values are kept. I have a fix for it already right now. I am about to PR it.

parkervcp commented 4 years ago

Try it now.

ccuetoh commented 4 years ago

Ok, that would work. But now you have a more serious bug. By default, Pterodactyl allows up to 6 entries (as far as I can see) per page.

Let's say I have 10 users registered. That means that if I query the panel to get all the users, I would get 6 users as a response and a paginations.links array of links that would point me to the second page to get the rest of the users. That holds true for all things that use pagination in Pterodactyl: Servers, Users, Clusters, and so on.

The current version of this library, and more so with this change can only show 6 of anything since it doesn't yet have any pagination support. That's what prompted me to create a new fork. I have found a sketchy workaround for this problem (https://github.com/CamiloHernandez/crocgodyl/commit/73b2da436ad7708da5b2476f1bdd12589b7ffd96#diff-d0b7f86cea13f90d3cc0cc2338da8d5fR113) and was in the process of working on a more robust solution.

ccuetoh commented 4 years ago

To clarify more on the topic of this bug: Links should absolutely be kept as It will be needed to implement pagination in the future.

parkervcp commented 4 years ago

Links don't need to be used at all. We know what page we are on and how many pages there are.

  "meta": {
    "pagination": {
      "total": 60,
      "count": 50,
      "per_page": 50,
      "current_page": 1,
      "total_pages": 2,
      "links": {
        "next": "http:\/\/gollum.parkervcp.com\/api\/application\/users?page=2"
      }
    }
prevPage = currentPage - 1
nextPage = currentPage + 1 

pagelink = <panel>/api/<endpoint>?page=<pageNumber>

this is overly simple as well.

I never use the page links. I only ever use pagination.total_pages.

ccuetoh commented 4 years ago

Yes, this holds true for Pterodactyl, but the links approach has 2 advantages:

Plus the links are going to be sent anyway. I do acknowledge though that using just the page count is simpler and quicker.

I propose that this solution (and PR) is accepted temporarily and a new issue is opened addressing the main problem of pagination.

parkervcp commented 4 years ago

This is a php issue and not a pterodactyl/crocgodyl issue. php's json_encode() function replaced empty json objects with an empty array.

As seen here - https://github.com/FriendsOfSymfony/FOSRestBundle/issues/980

ccuetoh commented 4 years ago

Though I understand what you're saying I been unable to recreate any instance in which Links is given as an array in the response. Even with a single-page pagination object, I get an empty object and not an empty array:

"meta": {
    "pagination": {
        "total": 1,
        "count": 1,
        "per_page": 50,
        "current_page": 1,
        "total_pages": 1,
        "links": {}
    }
 }

After switching the links in the Pagination struct from []Links to Links I have not run into any other unmarshalling errors.

parkervcp commented 4 years ago

I have many pages that are both in my test panel and it errors on single page if I have either []links or links{}

At this point the way I have it resolves the issue for all instances.