Fix complex packages import paths

[maxiride]

EDIT
Please skip directly to this reply as none of what I made matters anymore since I found another import path case which fails with this current PR.

See #462 and #255

Attempting to fix the parsing of complex package names by leveraging regex capabilities to capture the relevant parts.

After smashing my head for an hour I asked for help on Reddit, details of the regex expression can be found here, credits for the regex go to /u/Lee_Dailey.

I've also added the relevant test entries which used to fail and now they pass.

---

Notes

I've left the original code as commented out for reference, it should be removed if the PR is to be accepted to avoid polluting the code with unnecessary lines.

The regex expression can be easily batch tested here to ensure it satisfies package import names, I am not aware of any other strange import path other than gopkg.in/guregu/null.v3/zero. To nitpick the issue we should find other packages with complex import paths. I can't guarantee that this will work with every package ever.

Two assumptions on fully qualified names have been made to build the regex:

1. A type name is always in the form of pkgName.TypeName
2. The import path ends with the rightmost forward slash

This led to formulate these two regex:

• To match a package type: match everything backwards from the end of the string to the first encountered forward slash /
• To match a package name: match everything forward from the start of the string till the last dot found

[maxiride]

I now noticed that the same author (guregu) has another package with the import path gopkg.in/guregu/null.v3.

This import path, with the implementation in this PR will fail, given the go_type gopkg.in/guregu/null.v3.String this will yield:

• Captured type: null.v3.String ❌, actual working type is null.String
• Captured package name: gopkg.in/guregu/null.v3 ✅

--- FAIL: TestTypeOverrides/gopkg.in/guregu/null.v3.String (0.00s)
        config_test.go:130: type name mismatch;
              string(
            - 	"null.String",
            + 	"null.v3.String",
              )

---

---

New idea to address the issue

I am prone to believe that a foolproof approach could be to modify the config file structure adding separate fields for the import path and the type name so that we don't have to do any kind of guess work on how the package name is structured and how the type names are called.
This also imply that the end user would have full responsibility for a proper package and type declaration but I believe that to be acceptable.

If @kyleconroy can chime in I might attempt to implement this approach. However, being it a bit of an overhaul on several layers of the package I'd prefer to settle down with an accepted line of work before commencing anything.

[kyleconroy]

If @kyleconroy can chime in I might attempt to implement this approach. However, being it a bit of an overhaul on several layers of the package I'd prefer to settle down with an accepted line of work before commencing anything.

I'm going to need to think about the way forward a bit, so I suggest you pause work on the PR.

The issue is that we're trying to parse three parts of information from a single string: the import path, the package name, and the type identifier. I think you're correct that we need to use a struct for this instead of a string.

I also want to solve #177 at the same time.

My current proposal looks like this:

overrides:
  - go_type:
      import: "gopkg.in/guregu/null.v3"
      package: "null"
      type: "String"
    db_type: "string"
    "null": true

[maxiride]

I also want to solve #177 at the same time.

My current proposal looks like this:

overrides:
  - go_type:
      import: "gopkg.in/guregu/null.v3"
      package: "null"
      type: "String"
    db_type: "string"
    "null": true

Having explicit declaration of the three entries surely pinpoints what the user wants and helps in generating proper code. However, as of now, I'm not sure #177 could be solved by such approach, if I understood correctly the issue some kind of additional entry would be needed to mark the override to not be imported. Nonetheless seems to me that the structure you proposed is a good base starting point.

#413 on the other hand seems to me more easily addressable, if the config gets changed it could be an occasion to rethink the naming of the entry "null". As you already proposed either not_null or is_null is perfectly fine IMHO.

Since the null entry defaults to false I personally find that is_null is a more clearer take on the phraseology to use.

Here the same struct with the null field addressed.

overrides:
  - go_type:
      import: "gopkg.in/guregu/null.v3"
      package: "null"
      type: "String"
    db_type: "string"
    is_null: true

---

Overall, from my understanding of the package, addressing #255 and #413 at the same time is more easily done in this PR. #177 requires more brainstorming beyond the config struct itself.

[kyleconroy]

I think this solves #177 quite well. You just leave out the import and package declarations

overrides:
  - go_type:
      type: "State"
      column: "job.state"

[maxiride]

I've attempted to implement a solution, unit tests pass however, I am unable to proceed further in the testing of the new implementation due to an upstream issue #476.

I've made a custom Dockerfile to build the package in a Linux environment and while the package does build without errors I am unable to run any further integration test as debugging into a running docker container is a PITA.

I'll update when I have either a workaround or a working solution.

---

I guess I'll keep going with pushing the repo and inspect the GitHub tests

---

Took me long enough, eventually all tests passed 🥱😪, please beware that I am still unable to build the package and perform a proper test with the examples. If I build a custom image as follows I get an error I have a hard time to investigate using the docker container.

I couldn't stand leaving PR a step away from completeness. I eventually spun up a CT on my server to build and test the PR and can confirm that the PR works as expected you can check the authors example in internal/endtoend/testdata/complex_go_type

Awaiting a comment and review @kyleconroy

Recap of the changes made

New format of the YAML configuration file to use (fields structure and nesting is the same for JSON)

Note that this change also brings the replacement of the null tag with is_null

overrides:
  - go_type:
      import: "gopkg.in/guregu/null.v3"
      package: "null"
      type: "String"
    db_type: "string"
    is_null: true

This is reflected by the new struct GoTypeParams L155 in internal/config/config.go for proper unmarshalling of the configuration file.

Renamed GoTypeName and GoPackage

To avoid ambiguity on the use of GoTypeName and GoPackage they have been renamed, see here.
GoTypeName referred to the "fully qualified name to a type" used in the pre-PR status go_type, since we are now explicitly setting every field (import path, package name and type name) this IMHO led to confusion on what's referring to what.

Changed all relevant tests to comply with the new type structure

Note that TestTypeOverrides in config_test isn't needed anymore because we are declaring the fields explicitly and not extracting them as before, the test performed here checks that package name and type name are compliant to what is given in the config, but they will always be compliant because we are basically checking them against themselves.

---

In conclusion if everything works fine, before accepting the PR I'd like to squash several commits, furthermore I should probably change the PR branch, PRing to master is a bad idea, my bad but there is no development branch and I didn't know where else to PR.

Of course before proceeding a lot of commits need to be squashed and the example folder needs to be re-done completely with the new config schema and the consequent new models and queries etc.
I've re-done only the authors example for the sake of showing the results, the examples-complex-import should be removed aftwerwards.

[kyleconroy]

As I mentioned in a few comments, we can't change the type of the go_type parameter in a backwards-incompatible way. I'm happy to take over the PR from here if you don't want to implement backwards compatibility. Let me know.

[kyleconroy]

This is a backwards incompatible change. The GoTypeParams struct needs to work if go_type is a string. This will require a custom UnmarshalJSON method.

[maxiride]

As I mentioned in a few comments, we can't change the type of the go_type parameter in a backwards-incompatible way.

I didn't think about it but I stand corrected, it is indeed necessary.

I'm happy to take over the PR from here if you don't want to implement backwards compatibility. Let me know.

I will happily hand over the PR to be taken care of! =) Would have loved to contribute more but at this point I wouldn't even know how to proceed to implement backwards compatibility and I believe my PR is already "sloppy" enough for the junior dev I am. It's a task it's sincerely best left to you.

Checked the "Allow edits from maintainers" so you should be good to go.

---

I'm also sorry for the tons of commits I made which undoubtedly pollute the commit history, I've been more prone to a "commit often, stay safe" but I'm sure that before a merge most of them can be squashed.

[kyleconroy]

I'm also sorry for the tons of commits I made which undoubtedly pollute the commit history, I've been more prone to a "commit often,

No need to apologize. We squash commits for PRs, so all your commits just turn into one :)

[maxiride]

Any update about on which milestone we can expect this feature to be implemented?

[kyleconroy]

I'm hoping for the next release, 1.5.0. I've been focused on some large architecture changes, so this fell to the wayside. Sorry for the delay.

[maxiride]

No troubles! Just wanted an update :) glad to hear it might land in 1.5.0 Il lun 22 giu 2020, 17:28 Kyle Conroy <notifications@github.com> ha scritto:

…

I'm hoping for the next release, 1.5.0. I've been focused on some large architecture changes, so this fell to the wayside. Sorry for the delay. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA3WOCJZKF4YLTOSR27LJY3RX52CHANCNFSM4MR6SYHQ> .

[tv42]

You could just load the import path with https://godoc.org/golang.org/x/tools/go/packages and see what the name of the package is (the source is going to be available at sqlc generate time).

Or you could always explicitly name the import in the generated code, import foo "lots.of.dashes".

[tv42]

The code around this is a bit weird/neglected. This looks like it tries to massage the package name to strip common prefixes and suffixes, but then the carefully crafted typename is not used to set o.GoPackage. It seems to have confused type name vs package name...

I think it'd be a pretty easy refactor to discard all this logic and just explicitly name all non-stdlib imports.

https://github.com/kyleconroy/sqlc/blob/ba4c9bfc0bfdcb1616ca5ef9b1299f35b313c254/internal/config/config.go#L231-L244

[kyleconroy]

Sorry that this PR has languished. I'm heads down working on MySQL support. I promise this is next, but it's still going to be a while.

[maxiride]

Sorry that this PR has languished. I'm heads down working on MySQL support. I promise this is next, but it's still going to be a while.

It surely needs polishing or a complete rewrite using @tv42 suggested method.

For what's worth I've been using my fork with these modifications since April without issues so far.

[JessBellon]

Any update on this?
If its alright with y'all I can take a swing at a new PR written using @tv42's idea?

[maxiride]

Haven't heard anything from @kyleconroy but I honestly think the new proposed approach is waaaay smoother than my amateurish approach.

I'd be glad to see a polished and merged into master PR to address the issue. This PR "just works" but I do recognise is far from clean code. Please do go ahead.

[maxiride]

I'm closing the PR in favour of #757 that use the tools x/tools/packages offers which, in turn, brings to sqlc a more elegant, concise and clean solution to the issue. I hope to see it merged soon in sqlc, I've already switched to use the binary built from #757 branch.