voidpet / bugs

61 stars 9 forks source link

Attempting to overwrite pets returns internal error #39

Closed TheNewJavaman closed 2 years ago

TheNewJavaman commented 2 years ago

Consider returning a better error message in the 400 domain.

GraphQL input:

mutation UpdateMe($input: UserInput!) {
  updateMe(input: $input) {
    ... on FieldErrorResponse {
      field
      error
      __typename
    }
    ... on User {
      id
      username
      instagramUsername
      twitterUsername
      tiktokUsername
      youtubeChannelUrl
      discordInviteUrl
      merchPurchased
      pets {
        id
        name
        stage
        species
        ownerId
        __typename
      }
      __typename
    }
    __typename
  }
}

Json variables:

{
   "input":{
      "instagramUsername":"xxx",
      "tiktokUsername":"xxx",
      "twitterUsername":"",
      "youtubeChannelUrl":"",
      "discordInviteUrl":"",
      "pets": []
   }
}

Json response:

{
    "errors": [
        {
            "message": "Internal Error: ef28c01e-b312-4096-b5cc-39882b69fdc2"
        }
    ]
}
TheNewJavaman commented 2 years ago

Similar error if I attempt to overwrite my id. I assume this is a permissions issue, so I'm keeping it in this thread.

Json variables:

{
   "input":{
      "id": "0"
   }
}

Json response:

{
    "errors": [
        {
            "message": "Internal Error: c6abd414-2f1e-4de9-aa9c-cbaf896c4e72"
        }
    ]
}
TheNewJavaman commented 2 years ago
james-work-account commented 2 years ago

I agree with @TheNewJavaman - if ambiguity is important, returning a standard 400 code with a generic "BAD_REQUEST" JSON error would be reasonable. Even if it's a 401 or 403 to indicate "stop trying to make requests, stranger"...

In standard HTTP land, convention is that 4XX = user did something wrong and 5XX = something bad happened on the server that wasn't directly caused by bad user input. Ideally user inputs would be sanitised/validated on the server before anything heavy happens, so in theory no whacky requests would ever get to the point where they're returning 5XX's.

Either way, 200 doesn't make a lot of sense. Then again, it's meant to be an internal tool so 🤷‍♂️

benawad commented 2 years ago

this is how I want to api to function