Skip to content
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.

Sign up for GitHub

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

Fix: Pruning generated files for relative operations #2994

Merged
merged 6 commits into from
May 4, 2023
Merged

Fix: Pruning generated files for relative operations #2994

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4ca827b
Resolving symlinks in file paths
BobaFetters May 3, 2023
e061640
Adding test for symlink directories
BobaFetters May 3, 2023
fbcf014
Updated handling to check for and remove symlinks
BobaFetters May 3, 2023
fa6d427
Removing whitespace
BobaFetters May 3, 2023
9a98154
Refactoring symlink check
BobaFetters May 4, 2023
05b9e89
Refactored symlink resolution
BobaFetters May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion Sources/ApolloCodegenLib/ApolloCodegen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,9 +462,24 @@ public class ApolloCodegen {
default: break
}

return try globs.reduce(into: []) { partialResult, glob in
var existingPaths: Set<String> = try globs.reduce(into: []) { partialResult, glob in
partialResult.formUnion(try glob.match())
}

try resolveSymlinksInPaths(&existingPaths)

return existingPaths
}

private static func resolveSymlinksInPaths(_ paths: inout Set<String>) throws {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function looks like it's just removing symlink paths, not actually resolving the symlinks. Is that correct?

If so, we should probably change the name of the function. But there is also something awkward about this, because there is implicit knowledge that the paths returned from the glob have both the path with and without the symlink in the Set. Is that correct?

I'm feeling like we should be expressing this better and removing the need for implicit knowledge here. Especially since you are now calling resolvingSymlinksInPath() inside of the Glob code.

If we are going to resolve the symlinks in the Glob, I'm not even clear why we also need this here? I know we had talked about this being the responsibility of the Codegen, not the Glob implementation, but it feels like we are violating that anyways now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point renaming the function would make since. And yes, the issue currently is that both a real path and symlink path to a file exist in the Set which causes the deletion.

Also the reason resolvingSymlinksInPath() is called in Glob is specifically due to needing to make sure paths in tests are resolved from /private/var/.... to just /var/...., however that change would not actually fix the non-test issue because the directoryURL used at that point in the code would not actually be the symlinked path. The code I showed you earlier where we could do the symlink resolution in Glob.expand by building out the paths before adding them to the Set in the return could potentially solve for both the test and non-test issue, will have to check to verify, and if so we may just need to do that since it appears we will need some change inside of Glob anyways which is what we were trying to avoid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind if we do this in the Glob, but I feel like we need a clear definition of what we are actually doing.

The real issue here is one of separation of responsibilities. It's a current behavior that the output of the Glob has duplicated paths. That is – two paths that actually point to the same place because of symlinks. If we decide that is an acceptable behavior of Glob, because it is just used to pattern match the paths, and there are two paths there technically, then that's okay. Let's document that behavior and then deduplicate the paths in ApolloCodegen, as we are doing. In which case the fix for the unit tests /private/var needs a different fix probably.

I think it's also reasonable to say that the behavior of the Glob struct should do the resolving of symlinks and deduplication of paths, ensuring that its output only points to a single file one time, regardless of how many different paths are symlinked to the file's actual path. If we do that, we can fix the problem in one place, and we should be documenting (and probably testing) that as an intended and defined behavior of our Glob matching object.

I don't have a strong opinion on where we do this, only that we do it in a way that defines a responsibility of a single object that is sensible; is documented; is tested; and works in consistently in every case (of course bugs are bugs, so "every case" means "every known case").

I think I might be misunderstanding or missing some information here, but having the symlink deduplication in the codegen but also resolving symlinks in the Glob seems to be muddying the single responsibility principle here.

If there is a specific odd edge case where /private/var isn't being picked up as a symlink properly we can hard code in a fix for the edge case. But if we are going to resolve all symlinks lets have a clear definition of what that means, where it's happening, and why.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I built Glob to be a pattern matching engine; it expands given wildcards and outputs matching paths. IMO those should be resolved symlinks and there should not be any duplicates. Glob is at the crux of path discovery here and it feels that should be doing the work. I don't think there is ever a need for us to know both the symlink and actual path?

for path in paths {
let url = URL(fileURLWithPath: path).deletingLastPathComponent()
let resourceValues = try url.resourceValues(forKeys: [.isSymbolicLinkKey])
if let isSymbolicLink = resourceValues.isSymbolicLink,
isSymbolicLink {
paths.remove(path)
}
}
}

static func deleteExtraneousGeneratedFiles(
Expand Down
1 change: 1 addition & 0 deletions Sources/ApolloCodegenLib/FileGenerators/FileGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ extension FileGenerator {
) throws {
let directoryPath = target.resolvePath(forConfig: config)
let filePath = URL(fileURLWithPath: directoryPath)
.resolvingSymlinksInPath()
.appendingPathComponent(fileName.firstUppercased)
.appendingPathExtension(fileExtension)
.path
Expand Down
2 changes: 1 addition & 1 deletion Sources/ApolloCodegenLib/Glob.swift
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calvincestari I ended up refactoring the Glob code to this so that the same code in the compactMap can handle both production and test use cases instead of checking resource keys for production and then always still resolving symlinks. So just tweaking how the path is built allows for just resolving symlinks for every path that comes through.

Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ public struct Glob {
excludedSet.intersection(url.pathComponents).isEmpty
else { continue }

directories.append(url)
directories.append(url.resolvingSymlinksInPath())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code change in ApolloCodegen to resolve symlinks does not actually solve the issue of /private/var vs /var paths in test because neither is marked as a symlink. However, running .resolvingSymlinksInPath() does remove /private from paths when it exists, so to be able to run tests for this issue I also had to change this line.

Thoughts on this? Or other suggestions of maybe handling this?

}

} catch(let error) {
Expand Down
54 changes: 54 additions & 0 deletions Tests/ApolloCodegenTests/ApolloCodegenTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1446,6 +1446,60 @@ class ApolloCodegenTests: XCTestCase {
expect(ApolloFileManager.default.doesFileExist(atPath: testUserFileInChildPath)).to(beTrue())
expect(ApolloFileManager.default.doesFileExist(atPath: testUserFileInNestedChildPath)).to(beTrue())
}

func test__fileDeletion__inOperationRelativeDirectory__whenSymlinkIsUsed() throws {
// given
try createFile(containing: schemaData, named: "schema.graphqls")

let schemaDirectory = "SchemaModule"
let codeDirectory = "code"
let relativeSubPath = "Operations"
let operationFilename = "TestQuery.graphql"

try createOperationFile(
type: .query,
named: "TestQuery",
filename: operationFilename,
inDirectory: codeDirectory
)

let symLinkURL = directoryURL.appendingPathComponent("/\(codeDirectory)/\(relativeSubPath)/")
let symLinkDestURL = directoryURL.appendingPathComponent("\(schemaDirectory)/Sources/Operations/")
let fileValidationPath = symLinkDestURL.appendingPathComponent("\(operationFilename).swift").path

//setup symlink folder
try testFileManager.fileManager.createDirectory(at: symLinkDestURL, withIntermediateDirectories: true)
try testFileManager.fileManager.createSymbolicLink(at: symLinkURL, withDestinationURL: symLinkDestURL)

// when
let config = ApolloCodegenConfiguration.mock(
input: .init(
schemaSearchPaths: ["schema*.graphqls"],
operationSearchPaths: ["code/**/*.graphql"]
),
output: .init(
schemaTypes: .init(path: schemaDirectory,
moduleType: .swiftPackageManager),
operations: .relative(subpath: relativeSubPath)
),
options: .init(
pruneGeneratedFiles: true
)
)

// then

// running codegen multiple times to validate symlink related file creation/deletion bug
try ApolloCodegen.build(with: config, rootURL: directoryURL)
expect(ApolloFileManager.default.doesFileExist(atPath: fileValidationPath)).to(beTrue())

try ApolloCodegen.build(with: config, rootURL: directoryURL)
expect(ApolloFileManager.default.doesFileExist(atPath: fileValidationPath)).to(beTrue())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming here, without the code added in this PR, this test passes on the first expectation, fails the second, and passes the third, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct


try ApolloCodegen.build(with: config, rootURL: directoryURL)
expect(ApolloFileManager.default.doesFileExist(atPath: fileValidationPath)).to(beTrue())

}

func test__fileDeletion__givenGeneratedFilesExist_InOperationRelativeDirectoriesWithSubPath_deletesOnlyRelativeGeneratedFilesInOperationSearchPaths() throws {
// given
Expand Down
1 change: 1 addition & 0 deletions Tests/ApolloInternalTestHelpers/TestIsolatedFileManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public class TestIsolatedFileManager {
}

let filePath: String = fileDirectoryURL
.resolvingSymlinksInPath()
.appendingPathComponent(fileName, isDirectory: false).path

guard fileManager.createFile(atPath: filePath, contents: data) else {
Expand Down