Merge branch 'development' into if-it-smells-like-a-bundle-and-looks-like-a-bundle

app/src/lib/databases/repositories-database.ts

@@ -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()
+}

app/src/lib/markdown-filters/emoji-filter.ts

@@ -1,7 +1,7 @@
 import { INodeFilter } from './node-filter'
 import * as FSE from 'fs-extra'
 import { escapeRegExp } from '../helpers/regex'
-import uri2path from 'file-uri-to-path'
+import { fileURLToPath } from 'url'
 
 /**
  * The Emoji Markdown filter will take a text node and create multiple text and
@@ -122,7 +122,7 @@ export class EmojiFilter implements INodeFilter {
     if (cached !== undefined) {
       return cached
     }
-    const imageBuffer = await FSE.readFile(uri2path(filePath))
+    const imageBuffer = await FSE.readFile(fileURLToPath(filePath))
     const b64src = imageBuffer.toString('base64')
     const uri = `data:image/png;base64,${b64src}`
     this.emojiBase64URICache.set(filePath, uri)

app/src/lib/source-map-support.ts

@@ -1,7 +1,7 @@
 import * as Path from 'path'
 import * as Fs from 'fs'
-import fileUriToPath from 'file-uri-to-path'
 import sourceMapSupport from 'source-map-support'
+import { fileURLToPath } from 'url'
 
 /**
  * This array tells the source map logic which files that we can expect to
@@ -23,7 +23,7 @@ function retrieveSourceMap(source: string) {
 
   // We get a file uri when we're inside a renderer, convert to a path
   if (source.startsWith('file://')) {
-    source = fileUriToPath(source)
+    source = fileURLToPath(source)
   }
 
   // We store our source maps right next to the bundle

app/src/lib/stores/repositories-store.ts

@@ -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]')

app/test/unit/repositories-database-test.ts

@@ -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()
+  })
 })

app/yarn.lock

@@ -585,11 +585,6 @@ file-stream-rotator@^0.6.1:
   dependencies:
     moment "^2.29.1"
 
-file-uri-to-path@^2.0.0:
-  version "2.0.0"
-  resolved "https://registry.yarnpkg.com/file-uri-to-path/-/file-uri-to-path-2.0.0.tgz#7b415aeba227d575851e0a5b0c640d7656403fba"
-  integrity sha512-hjPFI8oE/2iQPVe4gbrJ73Pp+Xfub2+WI2LlXDbsaJBwT5wuMh35WNWVYYTpnz895shtwfyutMFLFywpQAFdLg==
-
 file-url@^2.0.2:
   version "2.0.2"
   resolved "https://registry.yarnpkg.com/file-url/-/file-url-2.0.2.tgz#e951784d79095127d3713029ab063f40818ca2ae"