PokeAPI / pokeapi

The Pokémon API
https://pokeapi.co
BSD 3-Clause "New" or "Revised" License
4.16k stars 937 forks source link

Issue when retrieving sprites with GraphQL #614

Closed simonorono closed 10 months ago

simonorono commented 3 years ago

Hi. I've been playing around with the GraphQL API and I can't find a way to retrieve Pokemon along with their sprites.

This is the query I'm using, but I'm getting every Sprite as null:

query pokemon {
  pokemon_v2_pokemonspecies(where: {pokemon_v2_pokemondexnumbers: {pokemon_v2_pokedex: {name: {_eq: "kanto"}}}}) {
    id
    name
    pokemon_v2_pokemons {
      pokemon_v2_pokemonsprites {
        sprites
      }
    }
  }
}

One result from this query is this:

{
  "data": {
    "pokemon_v2_pokemonspecies": [
      {
        "id": 1,
        "name": "bulbasaur",
        "pokemon_v2_pokemons": [
          {
            "pokemon_v2_pokemonsprites": [
              {
                "sprites": "{\"front_default\": null, \"front_female\": null, \"front_shiny\": null, \"front_shiny_female\": null, \"back_default\": null, \"back_female\": null, \"back_shiny\": null, \"back_shiny_female\": null, \"other\": {\"dream_world\": {\"front_default\": null, \"front_female\": null}, \"official-artwork\": {\"front_default\": null}}, \"versions\": {\"generation-i\": {\"red-blue\": {\"front_default\": null, \"front_gray\": null, \"back_default\": null, \"back_gray\": null}, \"yellow\": {\"front_default\": null, \"front_gray\": null, \"back_default\": null, \"back_gray\": null}}, \"generation-ii\": {\"crystal\": {\"front_default\": null, \"front_shiny\": null, \"back_default\": null, \"back_shiny\": null}, \"gold\": {\"front_default\": null, \"front_shiny\": null, \"back_default\": null, \"back_shiny\": null}, \"silver\": {\"front_default\": null, \"front_shiny\": null, \"back_default\": null, \"back_shiny\": null}}, \"generation-iii\": {\"emerald\": {\"front_default\": null, \"front_shiny\": null}, \"firered-leafgreen\": {\"front_default\": null, \"front_shiny\": null, \"back_default\": null, \"back_shiny\": null}, \"ruby-sapphire\": {\"front_default\": null, \"front_shiny\": null, \"back_default\": null, \"back_shiny\": null}}, \"generation-iv\": {\"diamond-pearl\": {\"front_default\": null, \"front_female\": null, \"front_shiny\": null, \"front_shiny_female\": null, \"back_default\": null, \"back_female\": null, \"back_shiny\": null, \"back_shiny_female\": null}, \"heartgold-soulsilver\": {\"front_default\": null, \"front_female\": null, \"front_shiny\": null, \"front_shiny_female\": null, \"back_default\": null, \"back_female\": null, \"back_shiny\": null, \"back_shiny_female\": null}, \"platinum\": {\"front_default\": null, \"front_female\": null, \"front_shiny\": null, \"front_shiny_female\": null, \"back_default\": null, \"back_female\": null, \"back_shiny\": null, \"back_shiny_female\": null}}, \"generation-v\": {\"black-white\": {\"front_default\": null, \"front_female\": null, \"front_shiny\": null, \"front_shiny_female\": null, \"back_default\": null, \"back_female\": null, \"back_shiny\": null, \"back_shiny_female\": null, \"animated\": {\"front_default\": null, \"front_female\": null, \"front_shiny\": null, \"front_shiny_female\": null, \"back_default\": null, \"back_female\": null, \"back_shiny\": null, \"back_shiny_female\": null}}}, \"generation-vi\": {\"omegaruby-alphasapphire\": {\"front_default\": null, \"front_female\": null, \"front_shiny\": null, \"front_shiny_female\": null}, \"x-y\": {\"front_default\": null, \"front_female\": null, \"front_shiny\": null, \"front_shiny_female\": null}}, \"generation-vii\": {\"ultra-sun-ultra-moon\": {\"front_default\": null, \"front_female\": null, \"front_shiny\": null, \"front_shiny_female\": null}, \"icons\": {\"front_default\": null, \"front_female\": null}}, \"generation-viii\": {\"icons\": {\"front_default\": null, \"front_female\": null}}}}"
              }
            ]
          }
        ]
      }
    ]
  }
}

Two things:

  1. all sprites are null
  2. rather than being part of the JSON response, the sprites are returned in a string containing another JSON document.

Am I doing something wrong or is it this a bug?

Naramsim commented 3 years ago

It's a bug. Currently, the sprites are the only non-working thing in the graphql engine. It's because we don't have a Postgres table for them.

brunocordioli072 commented 2 years ago

Is there any updates on the issue? I'm having the same problem...

Naramsim commented 2 years ago

No updates

I tried locally but then I had no time and didn't continue

MiroStW commented 2 years ago

shame :( otherwise its a great graphQL playground! 🙏

C-Garza commented 2 years ago

I ran the same query as @simonorono and I didn't get null for the sprites, I got the paths but the paths before they are serialized:

"sprites": "{\"front_default\": \"/media/sprites/pokemon/1.png\", ...}"

This is just how it appears inside the PokemonSprites model since that model is just a charfield. We don't use that media substring anywhere from what I can see, except maybe in a few tests. We could just entirely remove that media string and replace it with https://raw.githubusercontent.com/PokeAPI/sprites/master/ in the build file, because that's what we do in the serializers anyway. It'll make that table a bit bigger and it would still be a string, but at least the user could just parse it since it's stored as valid JSON.

simonorono commented 2 years ago

Tested with a fresh clone of the PokeAPI and can confirm this is no longer an issue.

Naramsim commented 2 years ago

Hi @simonorono, No implementation has been made. I feel the issue is still here.

Naramsim commented 2 years ago

@C-Garza , how could you possibly get the sprites' link?

simonorono commented 2 years ago

@Naramsim you're right, I was looking at the regular API :man_facepalming:.

C-Garza commented 2 years ago

@Naramsim I can't really recall, but if I remember correctly I think I made a mistake because I was doing it on my local machine and it had those sprite links after I ran some kind of step. But I'm pretty sure it shouldn't work still because the sprites don't have their own postgres table or something if I recall.

LeonEstrak commented 2 years ago

So i was able to reproduce this error and fix it on my machine. Since no PR has been raised against this, i am gonna assume this has not been explicitly fixed.

I downloaded the the tar ball from the releases page. Currently the latest is v2.3.0. The core issue seemed to be tar ball didn't to contain the sprites data at all. So when running the application, the sprites folder was empty, causing the null response. I manually added the sprites folder data, flushed the DB with make docker-flush-db , and rebuilt it with make docker-build-db. This fixed the issue and on using GraphQL Query mentioned above, i am getting something of an acceptable response.

Attaching gist for others to verify API response https://gist.github.com/LeonEstrak/cf6a288e45c35a2b516f7f762890952a

LeonEstrak commented 2 years ago

I ran the same query as @simonorono and I didn't get null for the sprites, I got the paths but the paths before they are serialized:

"sprites": "{\"front_default\": \"/media/sprites/pokemon/1.png\", ...}"

This is just how it appears inside the PokemonSprites model since that model is just a charfield. We don't use that media substring anywhere from what I can see, except maybe in a few tests. We could just entirely remove that media string and replace it with https://raw.githubusercontent.com/PokeAPI/sprites/master/ in the build file, because that's what we do in the serializers anyway. It'll make that table a bit bigger and it would still be a string, but at least the user could just parse it since it's stored as valid JSON.

@C-Garza I have raised https://github.com/PokeAPI/pokeapi/pull/727 to resolve this.

Naramsim commented 2 years ago

OK, thanks to @LeonEstrak we now have the URL of the sprites in the graphql response. They have to be parsed but at least they are there.

Thanks!

trev125 commented 2 years ago

Any update on these being parsed? I added some code on my side to parse the sprites object and then choose a specific one that I want, but it would be nice to handle that in the GQL query

Naramsim commented 2 years ago

No. sorry, updates here will require some actual work and nobody can right now

Minepatcher commented 1 year ago

Hello, I'm not sure if anyone is aware, but it looks like the URLs in sprites reverted to their previous '/media' form. These used to have the full URL in them at some point, but are now not working anymore. The only reason I noticed is because of a codepen I use this for. Just thought I'd let you know. Thank you.

Naramsim commented 1 year ago

You're right!

cybercris commented 1 year ago

Issue still happening. Going back to restfull right away

simonorono commented 1 year ago

image

Been playing with this and it seems that just changing the type of the column to jsonb would be enough to make Hasura return them properly. However, the API breaks and I haven't had the time to check what needs to be changed in the API for this to work.

Naramsim commented 1 year ago

image

Been playing with this and it seems that just changing the type of the column to jsonb would be enough to make Hasura return them properly. However, the API breaks and I haven't had the time to check what needs to be changed in the API for this to work.

Did you try to build the db with first cloning the submodules? The sprites need to be present locally

simonorono commented 1 year ago

image Been playing with this and it seems that just changing the type of the column to jsonb would be enough to make Hasura return them properly. However, the API breaks and I haven't had the time to check what needs to be changed in the API for this to work.

Did you try to build the db with first cloning the submodules? The sprites need to be present locally

I tried without cloning the submodules. Was just trying the format part of it.

SorraimiRivas commented 11 months ago

image

Am I doing it wrong or this is still broken ?

Naramsim commented 11 months ago

Still broken

SorraimiRivas commented 11 months ago

Is there any intention of fixing it? I see that it has been like this for quite a long time.

Naramsim commented 11 months ago

I tried to replicate it some time ago and at some point I managed to fix it. But I don't remember how now. It's gonna probably stay like this for a couple of months

LeonEstrak commented 11 months ago

@Naramsim I have raised #949 to fix this. Seems to be working fine while doing local testing.

image

I will see if i can get them parsed. If you could provide a little assistance here @simonorono, since it seems you got the parsing bug.

SorraimiRivas commented 11 months ago

@Naramsim I have raised #949 to fix this. Seems to be working fine while doing local testing.

image

I will see if i can get them parsed. If you could provide a little assistance here @simonorono, since it seems you got the parsing bug.

Many thanks !!

Naramsim commented 11 months ago

Hi the sprites should be there now. As a parsable string block.

JayCanuck commented 11 months ago

Hi the sprites should be there now. As a parsable string block.

Appears the parsable string blocks may be out-of-date compared to the REST API (eg. ogerpon, id 1017 returns all null values)

SorraimiRivas commented 11 months ago

Why does it differ so much to the way it is structured in the rest version?

JayCanuck commented 11 months ago

Ah, it looks like it's just Pokemon species 1011-1017 (specifically the new sprites added last week https://github.com/PokeAPI/sprites/pull/125) that are different in sprite values on GraphQL compared to REST (other related data for 1011-1017 looks equal). Looking through comments, I imagine this just means it's slightly behind and needs the update. Apologies for any confusion @Naramsim. Beyond those species, all string blocks, once parsed, align perfect. I understand that these matters take time; any idea when the graphql side might be able to get the update applied?

Naramsim commented 11 months ago

Why does it differ so much to the way it is structured in the rest version?

It's because of how our postgresql db is made. The "sprites" are a single entity and Hasura pulls out the value and serves it. The value is a big JSON.

SorraimiRivas commented 11 months ago

Why does it differ so much to the way it is structured in the rest version?

It's because of how our postgresql db is made. The "sprites" are a single entity and Hasura pulls out the value and serves it. The value is a big JSON.

I see, well, it is what it is I'll just stay from it then. Thanks.

C-Garza commented 11 months ago

I think @simonorono is right, if we change the column to JSONB then the JSON keys can be queried when converted with Hasura: https://hasura.io/blog/postgres-json-and-jsonb-type-support-on-graphql-41f586e47536/#schema.

If the REST Api does break, then probably some serializers have to be updated I'm guessing?

Naramsim commented 11 months ago

Can you try locally? 🙂

C-Garza commented 11 months ago

@Naramsim I've been trying to replicate it locally but I can't seem to get the JSON keys split up like @simonorono did 🤔 . I changed sprites = models.CharField(max_length=20000) to be sprites = JSONField(), which seems to use JSONB by default. I can see in the migration file that it does update it:

class Migration(migrations.Migration):

    dependencies = [
        ('pokemon_v2', '0013_pokemonabilitypast'),
    ]

    operations = [
        migrations.AlterField(
            model_name='pokemonsprites',
            name='sprites',
            field=django.contrib.postgres.fields.jsonb.JSONField(),
        ),
    ]

And in hasura, I can see the column is now a jsonb type, but it still appears as the long JSON string. Did you have to change something else @simonorono to get the keys to split?

simonorono commented 11 months ago

@Naramsim I've been trying to replicate it locally but I can't seem to get the JSON keys split up like @simonorono did 🤔 . I changed sprites = models.CharField(max_length=20000) to be sprites = JSONField(), which seems to use JSONB by default. I can see in the migration file that it does update it:

class Migration(migrations.Migration):

    dependencies = [
        ('pokemon_v2', '0013_pokemonabilitypast'),
    ]

    operations = [
        migrations.AlterField(
            model_name='pokemonsprites',
            name='sprites',
            field=django.contrib.postgres.fields.jsonb.JSONField(),
        ),
    ]

And in hasura, I can see the column is now a jsonb type, but it still appears as the long JSON string. Did you have to change something else @simonorono to get the keys to split?

@C-Garza I think you need to re-apply the Hasura metadata after changing the column type but it was so long ago I'm not sure.

C-Garza commented 11 months ago

Hmm, if it's running make hasura-apply then I did do that and it didn't split the keys up 🤔 . I'll have another look into it sometime this week.

simonorono commented 10 months ago

Hmm, if it's running make hasura-apply then I did do that and it didn't split the keys up 🤔 . I'll have another look into it sometime this week.

I figured it out and ended up creating the PR for it. The code is on PR #959. The issue was that during the building of the table we were inserting the JSON as a string, making Hasura not being able to parse it and return it properly.