Create method is not using user-provided ID

[Yoshiji]

Hello,

First, thanks for your contribution to the open source community with Golang tooling around AIP.

When using this lib, I noticed that a user-provided ID is not taken into account by the generated tests for the Create method.

From AIP-133:

An API must allow a user to specify the ID component of a resource (the last segment of the resource name) on creation [...] An API may allow the {resource}_id field have the field_behavior OPTIONAL, and generate a system-generated ID if one is not specified.

For example:

// Using user-specified IDs.
publishers/lacroix/books/les-miserables

// Using system-generated IDs.
publishers/012345678-abcd-cdef/books/12341234-5678-abcd

Which is the case for our API:

• user-provided ID matching the DNS naming constraint (basically /[a-z][0-9]-{4,64}/)
• if none provided, our system generates a uuid.

This lib doesn't seem to cover such case. I see the generated code looks like this:

msg, err := fx.service.CreateFoo(fx.ctx, &CreateFooRequest{
    Parent:     parent,
    Foo: fx.Create(parent),
})

But my CreateFooRequest has an extra FooId string optional field, and if set it must be used as the resource's ID.

Would it be possible to add support for this feature? If so, let me know how I can help.

Thanks

[thall]

Hi @Yoshiji ,

Strange that you don't get generated test code for that.

In this repo, CreateShipperRequest has an optional field shipper_id, which causes the generated testcases to supply user provided id, see testCreate .

Is your resource / create method different from our example (Shipper, CreateShipper)?

user-provided ID matching the DNS naming constraint (basically /[a-z][0-9]-{4,64}/)

We don't have a test for that, but it would be a great addition to the library :raised_hands:

[Yoshiji]

Hi @thall,

Thank you for the quick response. It was indeed a problem on my end: turns out you must define the singular on your message (like it is done in v1/shipper.proto#L19) because it is called in HasUserSettableIDField, and if that is an empty string then it won't find a match and therefore not generate the userSetID related code. That solves it! Thanks.

--

I do have other questions/suggestions, a mix and match of various small things. I'll post them here, but let me know if you prefer that I open another (or more) issue(s):

Item 1: On the Foo/Create/create_time test, I sometime get: assertion failed: expression is false: msg.CreateTime.AsTime().After(beforeCreate): msg.CreateTime (2024-10-11 18:47:29.455571 +0000 UTC) is not after beforeCreate (2024-10-11 18:47:29.462666 +0000 UTC (7ms difference). I am running postgres in a docker container using ory/dockertest, that might explain why the NOW() in postgres is not the same as time.Now() in golang. My plan is to Skip: []string{"Foo/Create/create_time"}, but I wondered if you guys encountered something similar on your end? (for us, we usually truncate time to the second in the tests). The same issue happens with Update/update_time.

Item 2: As a new user of this lib, I am struggling a bit to understand how it works, and often need to dig into the code. Would you be open to the idea of a PR with various small changes, mostly to improve developer experience?

Item 3:

user-provided ID matching the DNS naming constraint (basically /[a-z][0-9]-{4,64}/)

We don't have a test for that, but it would be a great addition to the library 🙌

Do you want me to (try to) do it?

[thall]

Great that you found the issue :+1:

Item 1: We have seen that issue as well, generally that problem has been for people who are running Mac, are you running Mac?

Item 2: Alright I see, do you have any examples of what was hard / confusing or any ideas what we can improve to make it easier?

Item 3: Yes sure, just put up a PR :rocket:

[Yoshiji]

Item 1: I am indeed running Mac 14.6.1 on a M2 Max chip. I haven't tested this in our CI (GitHub Actions), I'll test if the issue happens there as well and let you know.

Item 2: (see below)

Item 3: I have something cooking on my free time, expect a PR adding this later this week.

--

List of small things to change that would improve my dev exp and hopefully others':

1. find out what is actually failing a test: several generated assert calls are not including a human-friendly string as last argument describing the expectation. For example, it could include this message: assert(t, expectedCode, actualCode, "Method should fail with InvalidArgument if provided parent is invalid" in https://github.com/einride/protoc-gen-go-aip-test/blob/master/internal/aiptest/list/invalid_parent.go#L33 (which btw is the message stored in the Doc []string or that suite.Test).

2. put a ref of the offending AIP, on error, in the test output. For example, add (AIP-123) at the end of the assert output in case of failure.

3. use t.Helper() (https://pkg.go.dev/testing#B.Helper) in some generated intermediate funcs to avoid reporting the error on these but on lines in the t.Run blocks

4. add more details as comments for the Create/Update/Parents/IDGenerator funcs in the fields of the *SuiteConfig generated struct.

If you want, I could create a small PR applying the changes on a subset of the generated tests, to illustrate my suggestion, and with your review/comments/suggestions I can adapt and apply to the rest of the generated code.

[thall]

Item 1: I am indeed running Mac 14.6.1 on a M2 Max chip. I haven't tested this in our CI (GitHub Actions), I'll test if the issue happens there as well and let you know.

Please do :+1:

Item 3: I have something cooking on my free time, expect a PR adding this later this week.

I had some time today, and created this PR, please check if that fulfills your needs :raised_hands:

Item 2:

Great feedback! I totally agree with those! 1 and 2 I also think will help many people. Please submit PRs when you see an improvement, I will try to address your feedback!

Please notice that we found a bug related to user settable ids, see PR.