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

Upload avatar #167

Draft
wants to merge 38 commits into
base: main
Choose a base branch
from
Draft

Upload avatar #167

wants to merge 38 commits into from

Conversation

benjie
Copy link
Member

@benjie benjie commented May 15, 2020

This is a work in progress PR for adding avatar upload functionality, merging #101 and #108. It's no-where near ready.

  • Add createSignedUrl mutation
  • Add upload component
  • Render profile page in two sections - one for avatar, one for textual settings
  • Handle errors during avatar upload flow
  • Document how to configure IAM, S3, etc for uploads
  • Allow configuring the system to not allow avatar uploads (e.g. if you don't want the S3 faff)
  • Uploads should be private and expire automatically after a few hours
  • The uploaded URL should be sent to the server, validated, and then the object should be moved to public S3 that doesn't expire, and referenced in the DB so that it may later be safely deleted (beware of race conditions)
  • A trigger should be added such that when a new upload is performed, the old upload gets queued for deletion
  • Storage needs to be added to the database to track the uploads the user has permissions to delete

Unfortunately I timed out after 4 hours merging these PRs and attempting to implement the above functionality.

@jackbravo
Copy link
Contributor

jackbravo commented Jun 14, 2020

I tried running with a minio server, which you can run with something like:

mkdir /tmp/s3
docker run -p 9000:9000 --name minio1 \
  -v /tmp/s3:/data \
  minio/minio server /data

Then adding S3_UPLOADS_BUCKET=http://localhost:9000/uploads to my .env file.

But then, while trying to upload a file I get this error in the javascript console:

[GraphQL error]: message: syntax error at end of input, location: [{"line":2,"column":3}], path: ["createUploadUrl"]
Error: "GraphQL error: syntax error at end of input"
  code: "42601"
​  severity: "ERROR"

@lukeramsden
Copy link
Contributor

lukeramsden commented Jun 20, 2020

I got the same error as the previous commenter - I believe it is something to do with the SAVEPOINT in getCurrentUser not having a name, because changing the 3 queries to be [..] SAVEPOINT create_upload_url gives me a different error:

{
  "errors": [
    {
      "message": "SAVEPOINT can only be used in transaction blocks",
      "locations": [
        {
          "line": 25,
          "column": 3
        }
      ],
      "path": [
        "createUploadUrl"
      ],
      "extensions": {
        "exception": {
          "code": "25P01",
          "severity": "ERROR"
        }
      }
    }
  ],
  "data": {
    "createUploadUrl": null
  }
}

So when we call getCurrentUser it's not inside a transaction? That's weird because the docs say[0] that all mutations should run under a transaction. Could this be caused by using the wrong version of Postgraphile or Postgres? I'm using the latest version of this starter and Postgres 12

[0] https://www.graphile.org/postgraphile/make-extend-schema-plugin/#querying-the-database-inside-a-resolver

throw err;
}

const user = await getCurrentUser(context.rootPgPool);
Copy link
Contributor

@lukeramsden lukeramsden Jun 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is what is causing the input error issue, changing rootPgPool to pgClient fixes it. Seems that rootPgPool doesn't have the context necessary for app_public.current_user_id to work properly either.

@benjie
Copy link
Member Author

benjie commented Jun 20, 2020

Yes, the savepoint should have a name. The whole getCurrentUser function doesn't currently make sense; e.g. there's no point doing pool.query('savepoint...') because the client you pool.query('release savepoint...') from may not be the same client that created the savepoint.

Basically: this PR is a very WIP merge of two other PRs and is not suitable for usage - that's why it's in draft status.

@benjie benjie changed the base branch from master to main June 24, 2020 11:35
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

Successfully merging this pull request may close these issues.

5 participants