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

Add asset upload functionality #203

Merged
merged 15 commits into from
Sep 14, 2022
Merged

Add asset upload functionality #203

merged 15 commits into from
Sep 14, 2022

Conversation

ra0x3
Copy link
Contributor

@ra0x3 ra0x3 commented Sep 9, 2022

Description

  • This PR looks large but it isn't -- most of the diff is just moving code around (only 1K new lines)
    • Further about half of those added 1K lines are just queries 🥲
  • This PR does a few things:
    • Manually downgrades anyhow back to 1.53
    • Converts the database package into the fuel-indexer-database crate
      • Crate reservations (some of which we may not need) are here
    • Moves IndexerConfig logic from fuel-indexer-lib/lib.rs to fuel-indexer/config.rs
    • Moves the database components (IndexerConnection, IndexerConnectionPool, and the queries) from fuel-indexer-schema/{db, models}.rs to fuel-indexer-database/{lib, queries}.rs
      • Still a little more to move from fuel-indexer-schema into fuel-indexer-database (e.g., the database model structs, ForiengKey) but that can wait
    • Adds new migrations for index_asset_registry_* tables which will hold the uploaded assets
      • ⚠️ You'll need to check these tables when testing
    • Introduces an "identifier" to an index -- with the identifier just being the name of the index
      • The idea is that an index can be globally identified as $namesapce + $identifier
    • Adds placeholder authentication middleware
      • Will need to add some concept of assets "belonging to" an operator (i.e., address/wallet) before this can actually be used
    • Allows users to upload assets (manifest, schema, wasm)
    • Updates all relevant docs and examples

Some implementation details

⚠️ Testing steps are pretty painful/extensive -- happy to hop on a call and pair through this

  • Assets each kept in separate tables (schema, wasm, manifest)
  • When new assets are uploaded to the same index, versions are bumped (just a simple integer count increased by one)
  • An index uses the latest version of its assets
  • You can either upload all assets at once, or upload them individually
  • Service currently requires a restart after asset upload
    • With our EKS setup this isn't a problem
    • However, obviously the ideal case is not having to restart the service at all
      • Might require some logic to pass some state to the IndexerService 👀
  • If asset blob checksums/digests are the same (e.g., you're uploading a copy of the same blob) then no data is inserted
  • If no multipart is in the request -- you get a 405
  • If you include a multipart field that isn't schema, wasm, manifest -- you get a 405
  • No sqlx-data.json was generated from cargo sqlx prepare --database-url sqlite:///path-to/my.db 🤷

Testing steps

  • Stop your local postgres if you have one running
  • Spin up the end-to-end test bash tests/e2e/composable-indexer/compose-up.bash
  • Register a set of assets...(below command)
curl -v http://0.0.0.0:29987/api/index/counter/index1 \
    -F "manifest=@examples/counter/manifest.yaml" \
    -F "wasm=@target/wasm32-unknown-unknown/release/counter_indexer.wasm" \
    -F "schema=@examples/counter/schema/counter.graphql" \
    -H 'Content-type: multipart/form-data' -H "Authorization: foo" | json_pp
  • Check the relevant database tables to ensure your assets were uploaded
  • Try running that same curl command again -- you should still only see a single set of assets (duplicates aren't uploaded)
  • Now try to register a second set of assets (below command)
curl -v http://0.0.0.0:29987/api/index/composability_test/index1 \
    -F "manifest=@tests/e2e/composable-indexer/manifest.yaml" \
    -F "wasm=@target/wasm32-unknown-unknown/release/composable_indexer_lib.wasm" \
    -F "schema=@tests/e2e/composable-indexer/composable-indexer-lib/schema/schema.graphql" \
    -H 'Content-type: multipart/form-data' -H "Authorization: foo" | json_pp
  • You should see these assets uploaded successfully to a separate index
  • Now make some change to examples/counter/manifest.yaml then re-upload it using the below command
curl -v http://0.0.0.0:29987/api/index/counter/index1 \
    -F "manifest=@examples/counter/manifest.yaml" \
    -H 'Content-type: multipart/form-data' -H "Authorization: foo" | json_pp
  • You should see that you now have two assets for the counter.index1 index, and the latest version in index_asset_registry_manifest should be "2"
  • Stop the fuel-indexer Docker container
  • Start the fuel-indexer container again
  • Making a request to /ping should still work (because the indices were loaded from the database when the service was started again)

@ra0x3 ra0x3 self-assigned this Sep 9, 2022
@ra0x3 ra0x3 marked this pull request as draft September 9, 2022 22:35
This was linked to issues Sep 9, 2022
@ra0x3 ra0x3 linked an issue Sep 11, 2022 that may be closed by this pull request
@ra0x3 ra0x3 marked this pull request as ready for review September 12, 2022 23:39
fuel-indexer-schema/src/db/models.rs Outdated Show resolved Hide resolved
fuel-indexer/src/service.rs Show resolved Hide resolved
fuel-indexer/src/manifest.rs Outdated Show resolved Hide resolved
fuel-indexer/src/config.rs Outdated Show resolved Hide resolved
@ra0x3
Copy link
Contributor Author

ra0x3 commented Sep 14, 2022

@deekerno

  • Agree with all the feedback so far.
  • If there's no other feedback after a few days I'll merge as is and follow up in a small PR (so as to minimize the review noise since the feedback is pretty light).
  • If there is other feedback I'll just make the changes in this PR

deekerno
deekerno previously approved these changes Sep 14, 2022
tjsharp1
tjsharp1 previously approved these changes Sep 14, 2022
fuel-indexer-macros/src/parse.rs Show resolved Hide resolved
let mut conn = pool
.acquire()
.await
.expect("Failed to get database connection.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we return a 500 instead of panicking?

Copy link
Contributor Author

@ra0x3 ra0x3 Sep 14, 2022

Choose a reason for hiding this comment

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

I made an issue -- #215 -- cause this (failing gracefully) might take some time to figure out how to do properly

.layer(Extension(pool.clone()));

let asset_route = Router::new()
.route("/:namespace/:identifier", post(register_index_assets))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea here to have identifier as the graph name and namespace be something like an "org name"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll want a way of deconflicting name choices from different devs, something to inform them whether a name was taken, or help them choose a namespace name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tjsharp1

  • Yea that's the idea. namespace is your org, identifier is the name of that index running for that org
  • You're right we'll eventually want friendly messaging around what namespace's are available
    • We would first need to implement some actual form of user authentication so namespaces can be tied to authenticated users

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Prevent having to match database queries Support schema versioning Allow upgrading WASM blobs
3 participants