-
-
Notifications
You must be signed in to change notification settings - Fork 42
Refactor storage to use DriveManager, enabling testing #121
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
Changes from all commits
39750da
db4cec2
d5bfb8b
4916908
7d660e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,3 +3,4 @@ assets/ | |
| fedify-hollo-*.tgz | ||
| *.jsonl | ||
| node_modules/ | ||
| tmp/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,7 +53,7 @@ import { | |
| pinnedPosts, | ||
| posts, | ||
| } from "../../schema"; | ||
| import { disk, getAssetUrl } from "../../storage"; | ||
| import { drive } from "../../storage"; | ||
| import { extractCustomEmojis, formatText } from "../../text"; | ||
| import { type Uuid, isUuid } from "../../uuid"; | ||
| import { timelineQuerySchema } from "./timelines"; | ||
|
|
@@ -107,6 +107,7 @@ app.patch( | |
| }), | ||
| ), | ||
| async (c) => { | ||
| const disk = drive.use(); | ||
| const owner = c.get("token").accountOwner; | ||
| if (owner == null) { | ||
| return c.json( | ||
|
|
@@ -133,7 +134,7 @@ app.patch( | |
| contentLength: content.byteLength, | ||
| visibility: "public", | ||
| }); | ||
| avatarUrl = getAssetUrl(`${path}?${Date.now()}`, c.req.url); | ||
| avatarUrl = await disk.getUrl(path); | ||
|
Comment on lines
-136
to
+137
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not entirely sure what's intended here? Cache busting? It'd probably be better to just use random path then, not the same path every time?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it's for cache invalidation. Of course, we could use random path instead!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that'd probably be best. It does mean the old image is still in storage, and you'd probably want to clean that up eventually, but it'd avoid needing to append something to the URL generated by
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now, I think I'd accept that small regression that the cache isn't busted, and then address busting the cache separately. |
||
| } | ||
| let coverUrl = undefined; | ||
| if (form.header instanceof File) { | ||
|
|
@@ -156,7 +157,7 @@ app.patch( | |
| } catch (error) { | ||
| return c.json({ error: "Failed to upload header image." }, 500); | ||
| } | ||
| coverUrl = getAssetUrl(`${path}?${Date.now()}`, c.req.url); | ||
| coverUrl = await disk.getUrl(path); | ||
| } | ||
| const fedCtx = federation.createContext(c.req.raw, undefined); | ||
| const fmtOpts = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,51 +1,55 @@ | ||
| import { describe, test } from "node:test"; | ||
| import { describe, it } from "node:test"; | ||
| import type { TestContext } from "node:test"; | ||
|
|
||
| import app from "./index"; | ||
|
|
||
| describe("OAuth", async () => { | ||
| await test("GET /.well-known/oauth-authorization-server", async (t: TestContext) => { | ||
| t.plan(10); | ||
|
|
||
| const response = await app.request( | ||
| "http://localhost:3000/.well-known/oauth-authorization-server", | ||
| { | ||
| method: "get", | ||
| }, | ||
| ); | ||
|
|
||
| t.assert.equal(response.status, 200, "Should return 200-ok"); | ||
|
|
||
| const metadata = await response.json(); | ||
|
|
||
| t.assert.equal(metadata.issuer, "http://localhost:3000/"); | ||
| t.assert.equal( | ||
| metadata.authorization_endpoint, | ||
| "http://localhost:3000/oauth/authorize", | ||
| ); | ||
| t.assert.equal( | ||
| metadata.token_endpoint, | ||
| "http://localhost:3000/oauth/token", | ||
| ); | ||
| // Non-standard, mastodon extension: | ||
| t.assert.equal( | ||
| metadata.app_registration_endpoint, | ||
| "http://localhost:3000/api/v1/apps", | ||
| ); | ||
|
|
||
| t.assert.deepStrictEqual(metadata.response_types_supported, ["code"]); | ||
| t.assert.deepStrictEqual(metadata.response_modes_supported, ["query"]); | ||
| t.assert.deepStrictEqual(metadata.grant_types_supported, [ | ||
| "authorization_code", | ||
| "client_credentials", | ||
| ]); | ||
| t.assert.deepStrictEqual(metadata.token_endpoint_auth_methods_supported, [ | ||
| "client_secret_post", | ||
| ]); | ||
|
|
||
| t.assert.ok( | ||
| Array.isArray(metadata.scopes_supported), | ||
| "Should return an array of scopes supported", | ||
| ); | ||
| }); | ||
| describe("OAuth", () => { | ||
| it( | ||
| "Can GET /.well-known/oauth-authorization-server", | ||
| { plan: 10 }, | ||
| async (t: TestContext) => { | ||
| // We use the full URL in this test as the route calculates values based | ||
| // on the Host header | ||
| const response = await app.request( | ||
| "http://localhost:3000/.well-known/oauth-authorization-server", | ||
| { | ||
| method: "GET", | ||
| }, | ||
| ); | ||
|
|
||
| t.assert.equal(response.status, 200, "Should return 200-ok"); | ||
|
|
||
| const metadata = await response.json(); | ||
|
|
||
| t.assert.equal(metadata.issuer, "http://localhost:3000/"); | ||
| t.assert.equal( | ||
| metadata.authorization_endpoint, | ||
| "http://localhost:3000/oauth/authorize", | ||
| ); | ||
| t.assert.equal( | ||
| metadata.token_endpoint, | ||
| "http://localhost:3000/oauth/token", | ||
| ); | ||
| // Non-standard, mastodon extension: | ||
| t.assert.equal( | ||
| metadata.app_registration_endpoint, | ||
| "http://localhost:3000/api/v1/apps", | ||
| ); | ||
|
|
||
| t.assert.deepStrictEqual(metadata.response_types_supported, ["code"]); | ||
| t.assert.deepStrictEqual(metadata.response_modes_supported, ["query"]); | ||
| t.assert.deepStrictEqual(metadata.grant_types_supported, [ | ||
| "authorization_code", | ||
| "client_credentials", | ||
| ]); | ||
| t.assert.deepStrictEqual(metadata.token_endpoint_auth_methods_supported, [ | ||
| "client_secret_post", | ||
| ]); | ||
|
|
||
| t.assert.ok( | ||
| Array.isArray(metadata.scopes_supported), | ||
| "Should return an array of scopes supported", | ||
| ); | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we had like a
WEB_DOMAINorLOCAL_DOMAINsetup, like mastodon, then we could in theory calculate a default for this based on that domain. If in production, force https, else, http.That'd also give a better way to handle all the object IDs and such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to address this from Fedify first (which would require a new minor release).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, definitely. For now, making
STORAGE_URL_BASEmandatory works. We can relax that after you address this in Fedify.