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

POC: Generated model extra fields #2638

Merged
merged 1 commit into from
May 26, 2023
Merged

Conversation

auvn
Copy link
Contributor

@auvn auvn commented May 17, 2023

Whenever I was implementing something using gqlgen I was facing the need of several extra fields for intermediate generated go objects in order to pass different data from the parent resolver to the children. Yes, I can define a whole model as a separate object, but it's easier to append one or two extra fields instead.
This PR implements ability to define any extra fields for a generatable models. So they are available to consume by resolver implementations.

TLDR:
the following config

models:
  ExtraFieldsTest:
    extraFields:
      FieldInternalType:
        type: github.com/99designs/gqlgen/plugin/modelgen/internal/extrafields.Type
      FieldStringPtr:
        type: "string"
        isPointer: true
      FieldInt:
        type: "int64"
        overrideTags: 'json:"field_int_tag"'

turns out into the following generated model where extra fields are not exposed to the API consumers but only for resolvers.

type ExtraFieldsTest struct {
	SchemaField string `json:"SchemaField" database:"ExtraFieldsTestSchemaField"`
	// User defined extra field
	FieldInt int64 `json:"field_int_tag" database:"ExtraFieldsTestFieldInt"`
	// User defined extra field
	FieldInternalType extrafields.Type `json:"-" database:"ExtraFieldsTestFieldInternalType"`
	// User defined extra field
	FieldStringPtr *string `json:"-" database:"ExtraFieldsTestFieldStringPtr"`
}

Any suggestions about config improvements are appreciated.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@coveralls
Copy link

coveralls commented May 17, 2023

Coverage Status

Coverage: 79.152% (+0.05%) from 79.102% when pulling b2f8bf3 on auvn:extra-fields into 4d945da on 99designs:master.

@auvn
Copy link
Contributor Author

auvn commented May 21, 2023

@StevenACoffman Hi, Steven! What do you think about this tiny feature?
Your feedback is appreciated!

@StevenACoffman StevenACoffman merged commit 7ab3317 into 99designs:master May 26, 2023
@chadxzs
Copy link

chadxzs commented May 26, 2023

@auvn any chance for a follow-up with some docs about this feature?

@StevenACoffman
Copy link
Collaborator

Sorry for the delay! This is great! I would love some docs if you get a chance.

@auvn
Copy link
Contributor Author

auvn commented May 26, 2023

@auvn any chance for a follow-up with some docs about this feature?

Yeah, of course, I'll rise another PR for docs.

type ModelExtraField struct {
Type string `yaml:"type"`
IsPointer bool `yaml:"isPointer"`
OverrideTags string `yaml:"overrideTags"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would ideally like this to contain another string field Description that would set the goDoc comment for the field, if possible.

@StevenACoffman
Copy link
Collaborator

StevenACoffman commented May 26, 2023

@auvn We have an internal plugin that did a similar thing, in a worse way, but the config looked a little different:

  - name: ActivityKind
    type: "*string"
    description: |
      The activity kind of the sessions (e.g Exercise, Article).
  - name: CourseType
    type: "*string"
    description: |
      The course associated with the activity sessions (e.g. KMAP).

The delay in my reviewing (or even commenting) was that I was really trying to find the time to reconcile your code with that, but I just have been too slammed at my dayjob.

In that plugin, we support the builtin basic types (like string or int64), named types
(qualified by the full package path), pointers to those types (prefixed
with *), and slices of those types (prefixed with []).

For example, the following are valid types:

 string
 *github.com/Khan/webapp/pkg/web.Date
 []string
 []*github.com/Khan/webapp/pkg/web.Date

I very much like that in your implementation this functionality is just baked in to modelgen, rather than using a plugin that wraps modelgen.

However, If I could add one thing from our wrapper plugin version, is that over there the config supports setting the GoDoc comment for the generated field (Description).

I hacked up the internal plugins we have at Khan and stuck them in a public repo over here so you can see what I'm talking about (if that's helpful):

https://github.com/StevenACoffman/gqlgen-plugins/blob/main/extra_fields.go

@auvn
Copy link
Contributor Author

auvn commented May 26, 2023

@StevenACoffman you reference is good! In this case Im going to port the missing functionality and bake it in gqlgen.
this will be done in addition with docs update.

Thanks!

@StevenACoffman
Copy link
Collaborator

See follow-up #2655

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants