Add Forked From

[supersonicbyte]

This PR tries to resolve the following issue: #551

The forkedFrom attribute on the Repository will enable us to determine if the package is forked from another repo, and we can reflect it on the UI.

[daveverwer]

I'll take a proper look at this soon, but I noticed this as a first small issue. I wonder if it might be better to do this inside updateRepository. It would save us passing one more variable into that function as we already pass in metadata, which drives this logic.

I can give it a proper review tomorrow, but I thought I'd mention this first as it may change the structure of the PR a little bit.

[supersonicbyte]

Thanks for the feedback. It definitely makes sense to move this to the updateRepository. Will do that.

[daveverwer]

Just one note here missing from the PR description in case @finestructure takes a look and is missing the context. @supersonicbyte and I chatted and agreed this should be done in two PRs. One to populate the data and one to show it once populated. This is the first of the PRs.

[finestructure]

Thanks for the PR, @supersonicbyte, it'll be nice to have this show up on the page!

[finestructure]

We'll want to make sure to do a case-insensitive compare here:

Suggested change

	.filter(\.$url == parentUrl)
	.filter(\.$url, .custom("ilike"), parentUrl)

[supersonicbyte]

Good catch! Thanks.

[daveverwer]

Apologies for the slow review on this. I was convinced we had some code already that normalised a package URL, but I had a good look for it yesterday and couldn't find it, so it's possible it got removed.

I waited until this morning to merge this so I could keep an eye on it on staging. I'll merge it now and watch its progress over the next few hours.

Thanks so much for this contribution, @supersonicbyte! This is great. Let's chat about how to represent it in the UI next 👍 Thank you!

[finestructure]

Apologies for the slow review on this. I was convinced we had some code already that normalised a package URL, but I had a good look for it yesterday and couldn't find it, so it's possible it got removed.

We do over in the PackageList repo. I'd half written a reply to copy its URL based implementation and re-use it that way but didn't want to make this more complicated than necessary for @supersonicbyte . I've made a note to see if it makes sense and follow-up with a PR if it does.

[finestructure]

Sorry, slight tweak to my proposed getFork - we shouldn't pass in all of Metadata but instead use

func getFork(on database: Database, parent: Github.Metadata.Parent?) async throws -> Fork? {
    guard let parentUrl = parent?.normalizedURL else { return nil }

    if let packageId = try await Package.query(on: database)
        .filter(\.$url, .custom("ilike"), parentUrl)
        .first()?.id {
        return .parentId(packageId)
    } else {
        return .parentURL(parentUrl)
    }
}

with normalisation on the Parent instead of Github.Metadata.Repository:

private extension Github.Metadata.Parent {
    // Returns a normalized version of the URL. Adding a `.git` if not present.
    var normalizedURL: String? {
        guard let url else { return nil }
        guard !url.hasSuffix(".git") else { return url }
        let normalizedUrl = url + ".git"
        return normalizedUrl
    }
}

The test is also a bit nicer this way:

    func test_getFork() async throws {
        try await Package(id: .id0, url: "https://github.com/foo/parent.git".url, processingStage: .analysis).save(on: app.db)
        try await Package(url: "https://github.com/bar/forked.git", processingStage: .analysis).save(on: app.db)

        do {  // test lookup when package is in the index
            let fork = try await getFork(on: app.db, parent: .init(url: "https://github.com/foo/parent.git"))
            XCTAssertEqual(fork, .parentId(.id0))
        }

        do {  // test lookup when package is in the index but with different case in URL
            let fork = try await getFork(on: app.db, parent: .init(url: "https://github.com/Foo/Parent.git"))
            XCTAssertEqual(fork, .parentId(.id0))
        }

        do {  // test lookup when package is not in the index
            let fork = try await getFork(on: app.db, parent: .init(url: "https://github.com/some/other.git"))
            XCTAssertEqual(fork, .parentURL("https://github.com/some/other.git"))
        }

        do {  // test lookup when parent url is nil
            let fork = try await getFork(on: app.db, parent: nil)
            XCTAssertEqual(fork, nil)
        }
    }

[finestructure]

I believe this has been superseded by #3363 ?