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.

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

Does not delete old aliases if all aliases are removed #65

Open
jdhenke opened this issue Jul 8, 2019 · 1 comment
Open

Does not delete old aliases if all aliases are removed #65

jdhenke opened this issue Jul 8, 2019 · 1 comment

Comments

@jdhenke
Copy link

jdhenke commented Jul 8, 2019

What happened?

If you have aliases in your conjure definition and generated go code, then remove all aliases and regenerate the go code into the same directory, the old aliases.conjure.go file and alias definitions still exist when they should not.

$ cat /tmp/defs.yml
types:
  definitions:
    default-package: com.palantir
    objects:
      SomeObject:
        fields:
          someField: string
      SomeAlias:
  1 types:
        alias: string
$ ~/Downloads/conjure-4.6.1/bin/conjure compile /tmp/defs.yml /tmp/defs.json
$ go run main.go /tmp/defs.json --output /tmp/repro
$ cat /tmp/repro/com/palantir/aliases.conjure.go
// This file was generated by Conjure and should not be manually edited.

package palantir

type SomeAlias string
$ vi /tmp/defs.yml # remove alias
$ cat /tmp/defs.yml
types:
  definitions:
    default-package: com.palantir
    objects:
      SomeObject:
        fields:
          someField: string
$ ~/Downloads/conjure-4.6.1/bin/conjure compile /tmp/defs.yml /tmp/defs.json
$ go run main.go /tmp/defs.json --output /tmp/repro
$ cat /tmp/repro/com/palantir/aliases.conjure.go # THIS IS THE BUG -- these defs should no longer exist
// This file was generated by Conjure and should not be manually edited.

package palantir

type SomeAlias string

What did you want to happen?

conjure-go should delete the aliases.conjure.go if there are no aliases.

NOTE: This may apply to other files that conjure-go writes as well.

@nmiyake
Copy link
Contributor

nmiyake commented Jul 8, 2019

This is an issue that's somewhat known. The difficulty here is that conjure-go doesn't store state, so knowing that an existing file "should be deleted" isn't trivial.

The location of the output files (like aliases.conjure.go) is determined by the package path relative to the output directory. If a file in a particular package is no longer generated, then conjure-go doesn't know to look for the file.

The only approach I can think of is a process like the following:

When running the generation task:

  1. Crawl the entire output directory tree and determines all of the *.conjure.go files that exist
  2. Determine all of the *.conjure.go files that are generated by the task
  3. Remove all of the "extra" files (*.conjure.go files that were found but not generated by the task)
  4. (Maybe?) If removing the *.conjure.go files caused any directories to become empty based on that action, delete the directory, and do this recursively

Possible issues:

  1. If the output location is a large directory tree, crawling the tree may take time (for example, if the output was specified as /)
  • This is probably unlikely to occur, as generation targets are usually targeted directories
  • We could also impose some kind of cap on maximum number of directories/depth that can be visited (and bail out with an error or skip the delete operation if this is exceeded)
  1. If the output directory contains symlinks and they are followed, crawling the tree may take time
  • Can prevent by implementing in a manner that does not follow symlinks
  1. If multiple separate Conjure tasks/definitions generate into the same output directory, running the task for one definition will delete the output created/managed by another definition
  • I don't believe that this is a common scenario (or one we should even support), but it is a possibility

These possibles issues are probably much rarer than the common scenario outlined in this issue, so not opposed to implementing the fix. However, I think we may want to add a flag that allows this behavior to be disabled as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants