Skip to content

Commit

Permalink
Fix non-canonical crate name pages
Browse files Browse the repository at this point in the history
Before the Ember Data 5 update it was possible to visit `/crates/foo-bar` when the crate was actually called `foo_bar` and vice-versa. The Ember Data 5 update broke this behavior, and this commit is restoring it, by using `queryRecord()` instead of `findRecord()`, which can no longer deal with requesting a certain `id` and getting a different one returned from the server.
  • Loading branch information
Turbo87 committed Jan 13, 2025
1 parent 3f361f1 commit 85c637a
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 7 deletions.
30 changes: 25 additions & 5 deletions app/adapters/crate.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,23 @@ export default class CrateAdapter extends ApplicationAdapter {
coalesceFindRequests = true;

findRecord(store, type, id, snapshot) {
let { include } = snapshot;
// This ensures `crate.versions` are always fetched from another request.
if (include === undefined) {
snapshot.include = 'keywords,categories,downloads';
return super.findRecord(store, type, id, setDefaultInclude(snapshot));
}

queryRecord(store, type, query, adapterOptions) {
return super.queryRecord(store, type, setDefaultInclude(query), adapterOptions);
}

/** Removes the `name` query parameter and turns it into a path parameter instead */
urlForQueryRecord(query) {
let baseUrl = super.urlForQueryRecord(...arguments);
if (!query.name) {
return baseUrl;
}
return super.findRecord(store, type, id, snapshot);

let crateName = query.name;
delete query.name;
return `${baseUrl}/${crateName}`;
}

groupRecordsForFindMany(store, snapshots) {
Expand All @@ -22,3 +33,12 @@ export default class CrateAdapter extends ApplicationAdapter {
return result;
}
}

function setDefaultInclude(query) {
if (query.include === undefined) {
// This ensures `crate.versions` are always fetched from another request.
query.include = 'keywords,categories,downloads';
}

return query;
}
2 changes: 1 addition & 1 deletion app/routes/crate.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export default class CrateRoute extends Route {
let crateName = params.crate_id;

try {
return await this.store.findRecord('crate', crateName);
return await this.store.queryRecord('crate', { name: crateName });
} catch (error) {
if (error instanceof NotFoundError) {
let title = `${crateName}: Crate not found`;
Expand Down
16 changes: 15 additions & 1 deletion e2e/acceptance/crate.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { test, expect } from '@/e2e/helper';
import { expect, test } from '@/e2e/helper';

test.describe('Acceptance | crate page', { tag: '@acceptance' }, () => {
test('visiting a crate page from the front page', async ({ page, mirage }) => {
Expand Down Expand Up @@ -139,6 +139,20 @@ test.describe('Acceptance | crate page', { tag: '@acceptance' }, () => {
await expect(page.locator('[data-test-try-again]')).toBeVisible();
});

test('works for non-canonical names', async ({ page, mirage }) => {
await mirage.addHook(server => {
let crate = server.create('crate', { name: 'foo-bar' });
server.create('version', { crate });
});

await page.goto('/crates/foo_bar');

await expect(page).toHaveURL('/crates/foo_bar');
await expect(page).toHaveTitle('foo-bar - crates.io: Rust Package Registry');

await expect(page.locator('[data-test-heading] [data-test-crate-name]')).toHaveText('foo-bar');
});

test('navigating to the all versions page', async ({ page, mirage }) => {
await mirage.addHook(server => {
server.loadFixtures();
Expand Down
13 changes: 13 additions & 0 deletions tests/acceptance/crate-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,19 @@ module('Acceptance | crate page', function (hooks) {
assert.dom('[data-test-try-again]').exists();
});

test('works for non-canonical names', async function (assert) {
let crate = this.server.create('crate', { name: 'foo-bar' });
this.server.create('version', { crate });

await visit('/crates/foo_bar');

assert.strictEqual(currentURL(), '/crates/foo_bar');
assert.strictEqual(currentRouteName(), 'crate.index');
assert.strictEqual(getPageTitle(), 'foo-bar - crates.io: Rust Package Registry');

assert.dom('[data-test-heading] [data-test-crate-name]').hasText('foo-bar');
});

test('navigating to the all versions page', async function (assert) {
this.server.loadFixtures();

Expand Down

0 comments on commit 85c637a

Please sign in to comment.