xanzy / go-gitlab

GitLab Go SDK
Apache License 2.0
2.43k stars 961 forks source link

Visibility field is always empty #2002

Open janderssonse opened 2 months ago

janderssonse commented 2 months ago

Hello and thanks for all work with the library! I think a found a bug -> the visibility field is always empty. Tried the library on a few repositories and this seems consistent. Here is an example to illustrate:

package main

import (
    "fmt"

    "github.com/xanzy/go-gitlab"
)

func main() {
    git, _:= gitlab.NewClient("")

    gitlabProject,_, _ := git.Projects.GetProject("hanklank"+"/"+"quark-demo-api", nil)
    fmt.Println(gitlabProject)
}

image

svanharmelen commented 2 months ago

I would suggest to first determine if this is a "bug" of this package or a "feature" of the GitLab API 😉 Probably making the same API call using cURL and comparing the response will tell us which one it is.

heidiberry commented 2 months ago

The response from the GitLab API depends on whether the request is authenticated and what permissions the authenticated user has. In the case of the above, if you provide no token to the request, most of the fields listed in the API are not returned, including visibility:

{
    "id": 38582100,
    "description": null,
    "name": "quark-demo-api",
    "name_with_namespace": "Josef Andersson / quark-demo-api",
    "path": "quark-demo-api",
    "path_with_namespace": "hanklank/quark-demo-api",
    "created_at": "2022-08-14T20:27:24.727Z",
    "default_branch": "main",
    "tag_list": [],
    "topics": [],
    "ssh_url_to_repo": "git@gitlab.com:hanklank/quark-demo-api.git",
    "http_url_to_repo": "https://gitlab.com/hanklank/quark-demo-api.git",
    "web_url": "https://gitlab.com/hanklank/quark-demo-api",
    "readme_url": "https://gitlab.com/hanklank/quark-demo-api/-/blob/main/README.md",
    "forks_count": 0,
    "avatar_url": null,
    "star_count": 0,
    "last_activity_at": "2024-08-29T15:21:23.341Z",
    "namespace": {
        "id": 888681,
        "name": "Josef Andersson",
        "path": "hanklank",
        "kind": "user",
        "full_path": "hanklank",
        "parent_id": null,
        "avatar_url": "https://secure.gravatar.com/avatar/f1b08a3f2bc8cec7323d0e550d586a5a626c4cddd676887729de7717251c3736?s=80\u0026d=identicon",
        "web_url": "https://gitlab.com/hanklank"
    }
}

This results in the missing fields in the resulting Project object getting default values like an empty string or false for booleans. This is confusing as you wouldn't know if that was the real value or a consequence of it being missing from the response.

To make it clearer, the function could return a different version of the Project struct depending on the authentication status of the request. Or documentation could be added to explain the behaviour.

svanharmelen commented 2 months ago

Returning a different struct based on that kind of conditions is both hard (how to determine those conditions in this package) and not possible (in Go I don't see how I can return two different structs as these are methods so generics are not supported).

I think the only "real" solution would be to make all fields pointers, just like in the option structs. But that forces everyone to do nil checks continuously on each and every field someone want to access which I don't think is a very nice or ergonomic user experience.

So if this is inline with the results of the API (so it's not a bug) I'm not really open to change this for now.

janderssonse commented 2 months ago

Returning a different struct based on that kind of conditions is both hard (how to determine those conditions in this package) and not possible (in Go I don't see how I can return two different structs as these are methods so generics are not supported).

I think the only "real" solution would be to make all fields pointers, just like in the option structs. But that forces everyone to do nil checks continuously on each and every field someone want to access which I don't think is a very nice or ergonomic user experience.

Either way everyone will have to make check if there is a value or not. So the it does not matter much if it is pointers or not, even if one could argue that nil-values would be more natural (or not - it's arguable). But for example, I was expecting the Visibility field to always be present for public projects, with a value of "public", regardless of token given or not.

So if this is inline with the results of the API (so it's not a bug) I'm not really open to change this for now. Understandable. One small but easy improvement would be if a line was added to the documentation about the behaviour @heidiberry describs, as to not catch any other users by surprise.

Thanks to @heidiberry for the explaination and to @svanharmelen for the ongoing work. I suggest this issue could be closed if the behaviour was documented, as to help future users getting it right and avoid confusion, almost a one liner should be enough.