mirror of
https://github.com/desktop/desktop
synced 2024-10-02 14:23:59 +00:00
Merge pull request #13706 from desktop/preserve-login-case
Preserve login case
This commit is contained in:
commit
e23127a57b
|
@ -1,9 +1,15 @@
|
|||
import Dexie from 'dexie'
|
||||
import { BaseDatabase } from './base-database'
|
||||
import { WorkflowPreferences } from '../../models/workflow-preferences'
|
||||
import { assertNonNullable } from '../fatal-error'
|
||||
|
||||
export interface IDatabaseOwner {
|
||||
readonly id?: number
|
||||
/**
|
||||
* A case-insensitive lookup key which uniquely identifies a particular
|
||||
* user on a particular endpoint. See getOwnerKey for more information.
|
||||
*/
|
||||
readonly key: string
|
||||
readonly login: string
|
||||
readonly endpoint: string
|
||||
}
|
||||
|
@ -124,6 +130,7 @@ export class RepositoriesDatabase extends BaseDatabase {
|
|||
})
|
||||
|
||||
this.conditionalVersion(8, {}, ensureNoUndefinedParentID)
|
||||
this.conditionalVersion(9, { owners: '++id, &key' }, createOwnerKey)
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -158,3 +165,73 @@ async function ensureNoUndefinedParentID(tx: Dexie.Transaction) {
|
|||
.modify({ parentID: null })
|
||||
.then(modified => log.info(`ensureNoUndefinedParentID: ${modified}`))
|
||||
}
|
||||
|
||||
/**
|
||||
* Replace the case-sensitive [endpoint+login] index with a case-insensitive
|
||||
* lookup key in order to allow us to persist the proper case of a login.
|
||||
*
|
||||
* In addition to adding the key this transition will, out of an abundance of
|
||||
* caution, guard against the possibility that the previous table (being
|
||||
* case-sensitive) will contain two rows for the same user (only differing in
|
||||
* case). This could happen if the Desktop installation as been constantly
|
||||
* transitioned since before we started storing logins in lower case
|
||||
* (https://github.com/desktop/desktop/pull/1242). This scenario ought to be
|
||||
* incredibly unlikely.
|
||||
*/
|
||||
async function createOwnerKey(tx: Dexie.Transaction) {
|
||||
const ownersTable = tx.table<IDatabaseOwner, number>('owners')
|
||||
const ghReposTable = tx.table<IDatabaseGitHubRepository, number>(
|
||||
'gitHubRepositories'
|
||||
)
|
||||
const allOwners = await ownersTable.toArray()
|
||||
|
||||
const ownerByKey = new Map<string, IDatabaseOwner>()
|
||||
const newOwnerIds = new Array<{ from: number; to: number }>()
|
||||
const ownersToDelete = new Array<number>()
|
||||
|
||||
for (const owner of allOwners) {
|
||||
assertNonNullable(owner.id, 'Missing owner id')
|
||||
|
||||
const key = getOwnerKey(owner.endpoint, owner.login)
|
||||
const existingOwner = ownerByKey.get(key)
|
||||
|
||||
// If we've found a duplicate owner where that only differs by case we
|
||||
// can't know which one of the two is accurate but that doesn't matter
|
||||
// as it will eventually get corrected from fresh API data, we just need
|
||||
// to pick one over the other and update any GitHubRepository still pointing
|
||||
// to the owner to be deleted.
|
||||
if (existingOwner !== undefined) {
|
||||
assertNonNullable(existingOwner.id, 'Missing existing owner id')
|
||||
log.warn(
|
||||
`createOwnerKey: Conflicting owner data ${owner.id} (${owner.login}) and ${existingOwner.id} (${existingOwner.login})`
|
||||
)
|
||||
newOwnerIds.push({ from: owner.id, to: existingOwner.id })
|
||||
ownersToDelete.push(owner.id)
|
||||
} else {
|
||||
ownerByKey.set(key, { ...owner, key })
|
||||
}
|
||||
}
|
||||
|
||||
log.info(`createOwnerKey: Updating ${ownerByKey.size} owners with keys`)
|
||||
await ownersTable.bulkPut([...ownerByKey.values()])
|
||||
|
||||
for (const mapping of newOwnerIds) {
|
||||
const modified = await ghReposTable
|
||||
.where('[ownerID+name]')
|
||||
.between([mapping.from], [mapping.from + 1])
|
||||
.modify({ ownerID: mapping.to })
|
||||
|
||||
log.info(`createOwnerKey: ${modified} repositories got new owner ids`)
|
||||
}
|
||||
|
||||
await ownersTable.bulkDelete(ownersToDelete)
|
||||
}
|
||||
|
||||
/* Creates a case-insensitive key used to uniquely identify an owner
|
||||
* based on the endpoint and login. Note that the key happens to
|
||||
* match the canonical API url for the user. This has no practical
|
||||
* purpose but can make debugging a little bit easier.
|
||||
*/
|
||||
export function getOwnerKey(endpoint: string, login: string) {
|
||||
return `${endpoint}/users/${login}`.toLowerCase()
|
||||
}
|
||||
|
|
|
@ -3,6 +3,7 @@ import {
|
|||
IDatabaseGitHubRepository,
|
||||
IDatabaseProtectedBranch,
|
||||
IDatabaseRepository,
|
||||
getOwnerKey,
|
||||
} from '../databases/repositories-database'
|
||||
import { Owner } from '../../models/owner'
|
||||
import {
|
||||
|
@ -15,7 +16,7 @@ import {
|
|||
assertIsRepositoryWithGitHubRepository,
|
||||
isRepositoryWithGitHubRepository,
|
||||
} from '../../models/repository'
|
||||
import { fatalError, assertNonNullable } from '../fatal-error'
|
||||
import { fatalError, assertNonNullable, forceUnwrap } from '../fatal-error'
|
||||
import { IAPIRepository, IAPIBranch, IAPIFullRepository } from '../api'
|
||||
import { TypedBaseStore } from './base-store'
|
||||
import { WorkflowPreferences } from '../../models/workflow-preferences'
|
||||
|
@ -371,18 +372,22 @@ export class RepositoriesStore extends TypedBaseStore<
|
|||
}
|
||||
|
||||
private async putOwner(endpoint: string, login: string): Promise<Owner> {
|
||||
login = login.toLowerCase()
|
||||
const key = getOwnerKey(endpoint, login)
|
||||
const existingOwner = await this.db.owners.get({ key })
|
||||
let id
|
||||
|
||||
const existingOwner = await this.db.owners
|
||||
.where('[endpoint+login]')
|
||||
.equals([endpoint, login])
|
||||
.first()
|
||||
|
||||
if (existingOwner) {
|
||||
return new Owner(login, endpoint, existingOwner.id!)
|
||||
// Since we look up the owner based on a key which is the product of the
|
||||
// lowercased endpoint and login we know that we've found our match but it's
|
||||
// possible that the case differs (i.e we found `usera` but the actual login
|
||||
// is `userA`). In that case we want to update our database to persist the
|
||||
// login with the proper case.
|
||||
if (existingOwner === undefined || existingOwner.login !== login) {
|
||||
id = existingOwner?.id
|
||||
id = await this.db.owners.put({ id, key, endpoint, login })
|
||||
} else {
|
||||
id = forceUnwrap('Missing owner id', existingOwner.id)
|
||||
}
|
||||
|
||||
const id = await this.db.owners.add({ login, endpoint })
|
||||
return new Owner(login, endpoint, id)
|
||||
}
|
||||
|
||||
|
@ -465,8 +470,7 @@ export class RepositoriesStore extends TypedBaseStore<
|
|||
)
|
||||
: await Promise.resolve(null) // Dexie gets confused if we return null
|
||||
|
||||
const login = gitHubRepository.owner.login.toLowerCase()
|
||||
const owner = await this.putOwner(endpoint, login)
|
||||
const owner = await this.putOwner(endpoint, gitHubRepository.owner.login)
|
||||
|
||||
const existingRepo = await this.db.gitHubRepositories
|
||||
.where('[ownerID+name]')
|
||||
|
|
|
@ -1,6 +1,8 @@
|
|||
import {
|
||||
RepositoriesDatabase,
|
||||
IDatabaseGitHubRepository,
|
||||
IDatabaseOwner,
|
||||
getOwnerKey,
|
||||
} from '../../src/lib/databases'
|
||||
|
||||
describe('RepositoriesDatabase', () => {
|
||||
|
@ -37,4 +39,80 @@ describe('RepositoriesDatabase', () => {
|
|||
|
||||
await db.delete()
|
||||
})
|
||||
|
||||
it('migrates from version 8 to 9 by deleting duplicate owners', async () => {
|
||||
const dbName = 'TestRepositoriesDatabase'
|
||||
let db = new RepositoriesDatabase(dbName, 8)
|
||||
await db.delete()
|
||||
await db.open()
|
||||
|
||||
type OwnersModelBeforeUpgrade = Omit<IDatabaseOwner, 'key'>
|
||||
const ownersTableBeforeUpgrade = db.table<OwnersModelBeforeUpgrade, number>(
|
||||
'owners'
|
||||
)
|
||||
const endpoint = 'A'
|
||||
|
||||
const ownerA = await ownersTableBeforeUpgrade.add({
|
||||
endpoint,
|
||||
login: 'desktop',
|
||||
})
|
||||
const ownerB = await ownersTableBeforeUpgrade.add({
|
||||
endpoint,
|
||||
login: 'DeskTop',
|
||||
})
|
||||
|
||||
const originalRepoA: IDatabaseGitHubRepository = {
|
||||
ownerID: ownerA,
|
||||
name: 'desktop',
|
||||
private: false,
|
||||
htmlURL: 'http://github.com/desktop/desktop',
|
||||
defaultBranch: 'master',
|
||||
cloneURL: 'http://github.com/desktop/desktop.git',
|
||||
parentID: null,
|
||||
lastPruneDate: null,
|
||||
permissions: 'write',
|
||||
issuesEnabled: true,
|
||||
}
|
||||
const originalRepoB: IDatabaseGitHubRepository = {
|
||||
ownerID: ownerB,
|
||||
name: 'dugite',
|
||||
private: false,
|
||||
htmlURL: 'http://github.com/desktop/dugite',
|
||||
defaultBranch: 'master',
|
||||
cloneURL: 'http://github.com/desktop/dugite.git',
|
||||
parentID: null,
|
||||
lastPruneDate: null,
|
||||
permissions: 'write',
|
||||
issuesEnabled: true,
|
||||
}
|
||||
|
||||
const repoAId = await db.gitHubRepositories.add(originalRepoA)
|
||||
const repoBId = await db.gitHubRepositories.add(originalRepoB)
|
||||
|
||||
expect(await db.gitHubRepositories.count()).toEqual(2)
|
||||
expect(await db.owners.count()).toEqual(2)
|
||||
|
||||
db.close()
|
||||
|
||||
db = new RepositoriesDatabase(dbName, 9)
|
||||
await db.open()
|
||||
|
||||
expect(await db.gitHubRepositories.count()).toEqual(2)
|
||||
expect(await db.owners.count()).toEqual(1)
|
||||
|
||||
const migratedRepoA = await db.gitHubRepositories.get(repoAId)
|
||||
expect(migratedRepoA).toEqual(originalRepoA)
|
||||
|
||||
const migratedRepoB = await db.gitHubRepositories.get(repoBId)
|
||||
expect(migratedRepoB).not.toEqual(originalRepoB)
|
||||
|
||||
const migratedOwner = await db.owners.toCollection().first()
|
||||
|
||||
expect(migratedOwner).not.toBeUndefined()
|
||||
expect(migratedRepoA?.ownerID).toEqual(migratedOwner?.id)
|
||||
expect(migratedOwner?.endpoint).toEqual(endpoint)
|
||||
expect(migratedOwner?.key).toEqual(getOwnerKey(endpoint, 'DeskTop'))
|
||||
|
||||
await db.delete()
|
||||
})
|
||||
})
|
||||
|
|
Loading…
Reference in a new issue