Use fixed size integers

[ash2k]

I've noticed that a lot of the structs for JSON/API types use int. int is 32 bits on 32 bit systems and will not fit 64 bit values that are used for some fields (i.e. bigint in the DB). Also note that Rails by default uses 64 bit bigint for primary (and hence foreign) keys since 5.1.

I think there are at least these ways to address this issue:

• Change all ints to int64.
  • Pro: simple to implement the change
  • Pro: simple to maintain consistency - all integer columns are always represented as int64 type, even if they are 32 bit in the DB.
  • Pro (minor): future-proof. If GitLab updates the schema to use 64 bit instead of 32 bit for a field, code does not need to change.
  • Con: More memory consumption (does not matter as it's negligible)
• Change specific fields to int32 or int64 depending on the DB schema GitLab uses.
  • Con: it's hard to figure out how many bits each field needs from the DB schema because the schema is gigantic and is hard to navigate. Also, API response may not necessarily map to DB schema directly.
  • Con (minor): not future-proof.
• Use OpenAPI to generate structs for Go. I don't know if GitLab exposes an OpenAPI definition of its API, I've only found this issue https://gitlab.com/gitlab-org/gitlab/-/issues/16023

p.s. Kubernetes API conventions is relevant here - https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#primitive-types. It states:

All public integer fields MUST use the Go (u)int32 or Go (u)int64 types, not (u)int (which is ambiguous depending on target platform). Internal types may use (u)int.

[svanharmelen]

Hi @ash2k! While I think you technically have a valid point, I do wonder what actual issue you are trying to fix? Since the existence of this package (which is quite a while by now) there has not been a single report about anyone using the package having an actual issue (bug or error) due to this.

That fact, together with the fact that this will be a backwards incompatible change, gives me the feeling we should not proactively change this right now. I would like to wait until this actually starts causing issues for users of this package (which I doubt will ever happen). Once it does, it's of course clear it should be addressed and in that case I would go for the first option and just update all int fields to int64 to keep both the code and the usage consistent.

There was another issue opened not too long ago (#875) which also was a nice technical improvement, but was also hitting a lot or struct fields, I mention that one here on purpose as whenever (if) this becomes relevant we should probably also make that change at the same time (reducing the impact for users of this package).

[ash2k]

Hi @svanharmelen. The only real issue here is big numbers will not work for 32 bit clients. In practice most numbers are smaller than 2^31 and most code is 64 bit. I do agree this is a breaking change that is probably not worth it. Just wanted to document this at an issue and bring to your attention. Perhaps new fields can be int64.

[svanharmelen]

Just wanted to document this at an issue and bring to your attention

Thanks for that!

Perhaps new fields can be int64

I prefer to keep this consistent, so change all or change none. But let's keep this issue open so we don't forget about it and when another bigger release makes sense we can maybe add this one as well.

Thanks!

[ash2k]

I very much agree inconsistencies are annoying and it's not worth it. Cheers!

[Unit193]

This is presumably what's causing test failures on 32bit arches now, which is so far preventing it from being in the next stable release of Debian.

A log of the errors can be viewed on ci.debian.org