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

Add delete file support to CreateTree #1307

Closed
wants to merge 10 commits into from

Conversation

@helmus
Copy link

helmus commented Oct 11, 2019

This solves #1268

Deleting a file trough the CreateTree operation right now is not possible trough this SDK because it is not possible to have the SHA field serialized to json null due to the omitempty struct flag.

I propose a separate type TreeDeleteEntry so the delete operation can be made explicit.
It's not possible to remove the omitempty struct tag on the existing type because this will cause a conflict with the content field when making the api call.

I may add tests later but just wanted to point this out at least.

@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 11, 2019

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no label Oct 11, 2019
@helmus

This comment has been minimized.

Copy link
Author

helmus commented Oct 11, 2019

@googlebot I signed it!

@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 11, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Oct 11, 2019
@helmus helmus changed the title add delete support Add file delete support to CreateTree Oct 11, 2019
@helmus helmus changed the title Add file delete support to CreateTree Add delete file support to CreateTree Oct 11, 2019
@helmus helmus force-pushed the helmus:feature/tree-delete branch from b3bc335 to 3bf0f50 Oct 11, 2019
@helmus helmus force-pushed the helmus:feature/tree-delete branch from 3bf0f50 to ed657cf Oct 11, 2019
@gmlewis

This comment has been minimized.

Copy link
Collaborator

gmlewis commented Oct 12, 2019

I'm sorry, @helmus, but I'm not happy with this approach.

Any time strongly-typed structs are replaced by []interface{}, some careful thought needs to go into why this is being done.

You mentioned in the description that

it is not possible to have the SHA field serialized to json null due to the omitempty struct flag.

In Go, we typically solve this by making the string a pointer, and then sending an explicit String("").

However, I'm wondering if we should first document how #1268 can be solved using curl first and then figure out how that translates to this Go client repo without sacrificing the nice data structs that have been built up.

Maybe @gauntface has other thoughts or comments about this.

@helmus

This comment has been minimized.

Copy link
Author

helmus commented Oct 12, 2019

@gmlewis totally understand your point. I'm also not a fan of interface{} collections.

The correct json structure to delete a file would look like this.

{
    "path": "file.rb",
    "mode": "100644",
    "type": "blob",
    "sha":  null
}

However, if we where to pass in string("") for sha in the TreeEntry struct, we would get the following:

{
    "path": "file.rb",
    "mode": "100644",
    "type": "blob"
}

And the github api will reject this as it requires an explicit null.
One way to solve it might be by implementing a custom Marshaler to differentiate between nil pointer and an empty string. This is a fairly implicit solution, and would have to be well documented, as it deviates from standard go struct tag marshaling.
Another approach could maybe be to define a common interface for the two different types that simply returns a base type. So you can be a little bit more specific for the collection type. Similar to this approach

Happy to update the code to an approach that suits you more and add tests to it.
The main issue is that deleting now is not possible. We can not simply remove the omitempty tag from the SHA field on the existing TreeEntry type, because that would conflict with the Content field.

@gmlewis

This comment has been minimized.

Copy link
Collaborator

gmlewis commented Oct 12, 2019

OK, I think I understand. What about creating a separate DeleteTree method that passes the appropriate fields without the changing the current CreateTree functionality?

@helmus

This comment has been minimized.

Copy link
Author

helmus commented Oct 12, 2019

@gmlewis if we do that, then we'll lose the ability to delete and modify files in the same commit.

I pushed a new approach in d8aa19d

Here I defined a new type TreeEntryBase which contains the common fields that define a TreeDeleteEntry and TreeEntry.
Using this type we can define an interface ITreeEntry which allows us to maintain more reasonable type safety in the CreateTree method.

However this does break backwards compatibility. As go won't allow implicit type conversions between arrays.

We could also leave CreateTree untouched, ( maybe deprecate it ) and define a new function CreateITree. But then deleting would only be possible trough CreateITree, which won't be that obvious. Curious to hear what you think.

@gmlewis

This comment has been minimized.

Copy link
Collaborator

gmlewis commented Oct 13, 2019

I can't review right now, but just a quick comment that this sounds like a good approach. Also, it is OK to break the API because we use semantic versioning in this repo (although we obviously wish to minimize breaking changes except for good reasons which I believe this is one).

@gmlewis

This comment has been minimized.

Copy link
Collaborator

gmlewis commented Oct 13, 2019

I haven't reviewed the PR yet, @helmus, but it looks like the unit tests are passing failing (doh!) with this error:

=== RUN   TestGitService_CreateTree
--- FAIL: TestGitService_CreateTree (0.00s)
    git_trees_test.go:79: Git.CreateTree request body: &{BaseTree:b Entries:[<nil>]}, want &{BaseTree:b Entries:[github.TreeEntry{SHA:"7c258a9869f33c1e1e1f74fbb32f07c86cb5a75b", Path:"file.rb", Mode:"100644", Type:"blob"}]}
=== RUN   TestGitService_CreateTree_Content
--- FAIL: TestGitService_CreateTree_Content (0.00s)
    git_trees_test.go:141: Git.CreateTree request body: &{BaseTree:b Entries:[<nil>]}, want &{BaseTree:b Entries:[github.TreeEntry{Path:"content.md", Mode:"100644", Content:"file content"}]}

You should be able to duplicate the problem locally by running go test ./.... See CONTRIBUTING.md for more details.

helmus added 2 commits Oct 13, 2019
Path: itemAsJson.GetString("path"),
Mode: itemAsJson.GetString("mode"),
Type: itemAsJson.GetString("type"),
Size: itemAsJson.GetInt("size"),
Content: content,
URL: itemAsJson.GetString("url"),
Comment on lines +168 to +173

This comment has been minimized.

Copy link
@helmus

helmus Oct 13, 2019

Author

This now breaks the contract with the struct tags on TreeEntry and TreeDeleteEntry, reflection would need to be used in order to get the correct struct tags. I think it's probably fine since A: createTree is a private type, B: createTree unmarshalling is only used in tests ( <- this is my assumption )

helmus added 3 commits Oct 13, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 13, 2019

Codecov Report

Merging #1307 into master will increase coverage by 0.14%.
The diff coverage is 87.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1307      +/-   ##
==========================================
+ Coverage   73.42%   73.57%   +0.14%     
==========================================
  Files          86       86              
  Lines        6047     6111      +64     
==========================================
+ Hits         4440     4496      +56     
- Misses        837      841       +4     
- Partials      770      774       +4
Impacted Files Coverage Δ
github/git_trees.go 87.09% <87.69%> (+0.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 740806a...1bcf54e. Read the comment docs.

helmus added 3 commits Oct 13, 2019
@gmlewis

This comment has been minimized.

Copy link
Collaborator

gmlewis commented Oct 13, 2019

@helmus - this is looking way more complicated than I hoped the solution would be.

Can we please take a step back for a moment and give an example of how someone would currently delete FileA and modify FileB within the same commit using curl and the GitHub v3 Developer API?

@helmus

This comment has been minimized.

Copy link
Author

helmus commented Oct 14, 2019

I think we can probably simplify the approach by implementing custom marshaling and unmarshaling logic for just the TreeEntry struct, which would deviate a little from standard go struct tag marshaling logic but it would get rid of the type hierarchy and is a little bit more in line with the api's interface.

The complication with this approach is caused because we lose the type information when serialization objects of type TreeDeleteEntry and TreeEntry, and the tests expect the json objects to be serialized back into in ITreeEntry array.

When serializing the json objects back to an array of ITreeEntry go can not decide if the type of the json object is actually a TreeDeleteEntry or TreeEntry, as there is no explicit type information ( it could unmarshal to either one ).
Hence I implemented the UnmarshalJSON function for createTree in order to differentiate between TreeDeleteEntry and TreeEntry during unmarshalling.
We can determine if the json object is a TreeDeleteEntry if both the sha and content field are empty / missing.

I created this repo to create this commit trough curl:
helmus/go-github-example@caa51d1
Here are the curl examples, I hope this clarifies it a little more for you.

Request files

create_tree.json
----------------
{
  "base_tree": "ff8e70e80b8639df2483ba6c9faaa9b38094eb15",
  "tree": [
    {
      "path": "FileA",
      "mode": "100644",
      "type": "blob",
      "sha": null
    },
    {
      "path": "FileB",
      "mode": "100644",
      "type": "blob",
      "content": "updating this content !"
    }
  ]
}

commit.json
----------------
{
  "message": "Delete and update file",
  "author": {
    "name": "willem dhaeseleer",
    "email": "dhwillem@gmail.com",
    "date": "2019-01-09T16:13:30+12:00"
  },
  "parents": [
    "8db90c2b21894bef2375a86fdd1074e64ea7340c"
  ],
  "tree": "09d88eece5f37ada28978dfd814a7417da9dd62a"
}

Request responses:

curl -s -X POST -d @create_tree.json -H "Authorization: token $MYTOKEN" https://api.github.com/repos/helmus/go-github-example/git/trees

// Note that the api does not actually return the deleted files in tree, so there is no inherent need to ever deserialize to a `TreeDeleteEntry` struct. 
// However, we do need to deserialize to an ITreeEntry type, which requires specifying a concrete type.

{
  "sha": "09d88eece5f37ada28978dfd814a7417da9dd62a",
  "url": "https://api.github.com/repos/helmus/go-github-example/git/trees/09d88eece5f37ada28978dfd814a7417da9dd62a",
  "tree": [
    {
      "path": "FileB",
      "mode": "100644",
      "type": "blob",
      "sha": "de887b15e780940c2bc2d2334b80e465a1fadc96",
      "size": 23,
      "url": "https://api.github.com/repos/helmus/go-github-example/git/blobs/de887b15e780940c2bc2d2334b80e465a1fadc96"
    },
    {
      "path": "README.md",
      "mode": "100644",
      "type": "blob",
      "sha": "09a00deca4dee4d75b3173334670361c881d7160",
      "size": 19,
      "url": "https://api.github.com/repos/helmus/go-github-example/git/blobs/09a00deca4dee4d75b3173334670361c881d7160"
    }
  ],
  "truncated": false
}

curl -s -X POST -d @commit.json -H "Authorization: token $MYTOKEN" https://api.github.com/repos/helmus/go-github-example/git/commits

{
  "sha": "caa51d1f3cc553efa6064960d0730c4abc09c8b3",
  "node_id": "MDY6Q29tbWl0MjE0OTExODM4OmNhYTUxZDFmM2NjNTUzZWZhNjA2NDk2MGQwNzMwYzRhYmMwOWM4YjM=",
  "url": "https://api.github.com/repos/helmus/go-github-example/git/commits/caa51d1f3cc553efa6064960d0730c4abc09c8b3",
  "html_url": "https://github.com/helmus/go-github-example/commit/caa51d1f3cc553efa6064960d0730c4abc09c8b3",
  "author": {
    "name": "willem dhaeseleer",
    "email": "dhwillem@gmail.com",
    "date": "2019-01-09T04:13:30Z"
  },
  "committer": {
    "name": "willem dhaeseleer",
    "email": "dhwillem@gmail.com",
    "date": "2019-01-09T04:13:30Z"
  },
  "tree": {
    "sha": "09d88eece5f37ada28978dfd814a7417da9dd62a",
    "url": "https://api.github.com/repos/helmus/go-github-example/git/trees/09d88eece5f37ada28978dfd814a7417da9dd62a"
  },
  "message": "Delete and update file",
  "parents": [
    {
      "sha": "8db90c2b21894bef2375a86fdd1074e64ea7340c",
      "url": "https://api.github.com/repos/helmus/go-github-example/git/commits/8db90c2b21894bef2375a86fdd1074e64ea7340c",
      "html_url": "https://github.com/helmus/go-github-example/commit/8db90c2b21894bef2375a86fdd1074e64ea7340c"
    }
  ],
  "verification": {
    "verified": false,
    "reason": "unsigned",
    "signature": null,
    "payload": null
  }
}


@gmlewis

This comment has been minimized.

Copy link
Collaborator

gmlewis commented Oct 14, 2019

Thank you for the detailed example, @helmus!

Now, can you please explain to me again why we can't simply remove the omitempty on the SHA, like this:
https://play.golang.org/p/6WpjlsJf9ii
?

What does this break?

And if it breaks something, can we come up with a simple way to optionally include or remove the SHA field?

@helmus

This comment has been minimized.

Copy link
Author

helmus commented Oct 14, 2019

@gmlewis

This comment has been minimized.

Copy link
Collaborator

gmlewis commented Oct 14, 2019

I'm wondering if we can piece together a struct using an array of json.RawMessage to build up the correct JSON? I'm going to have to think some more about this... I think there must be a simple an clean way to do it. Thank you, @helmus!

@helmus

This comment has been minimized.

Copy link
Author

helmus commented Oct 14, 2019

@helmus

This comment has been minimized.

Copy link
Author

helmus commented Oct 14, 2019

closing in favor #1310

@helmus helmus closed this Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.