Skip to content

Conversation

@abemedia
Copy link
Contributor

@abemedia abemedia commented May 20, 2025

Add support for the extension x-ogen-type. E.g. x-ogen-type: github.com/shopspring/decimal.Decimal will result in the Go type of the schema being decimal.Decimal.

A few gotchas regarding the use:

  • Types specified like this will be checked to see what interfaces they implement (e.g. github.com/ogen-go/ogen/json.Marshaler, github.com/ogen-go/ogen/json.Unmarshaler, json.Marshaler, json.Unmarshaler, encoding.TextMarshaler, encoding.TextUnmarshaler, encoding.BinaryMarshaler, encoding.BinaryUnmarshaler) and will be (un)marshaled using these interfaces. If they don't implement any of them it will fall back to using the Marshal and Unmarshal functions from stdlib json package.
  • If different external types have the same package name they will be aliased by adding integers e.g. decimal, decimal2, decimal3 etc.
  • If multiple types have the same name, creating generics (optional, nullable) will fail due to the naming conflicts and require using x-ogen-type.
  • Pointers are not currently supported. For now a pointer type (e.g. *pkg.Type) will result in a non-pointer (e.g. pkg.Type). I made an attempt at adding support for pointers but it got pretty complex pretty quickly and it wouldn't be possible to simply hijack PrimitiveType to achieve this. I did also try wrapping the type in ir.Pointer but for optional pointers this results in a pointer to a pointer (e.g. **pkg.Type) which I feel is just as bad as using a value instead of a pointer. If you think this is a requirement I can add a new type KindExternal with custom templates to handle this.
  • Validation is not supported for external types.
  • I added support for builtin types (e.g. string, int etc.) but on second thought, do we even want this? All builtin types can be generated using standard schema.

This PR also fixes an issue where setting default values for aliases was broken if the alias had a format (e.g. date-time, ip etc.)

Closes #1335

@abemedia abemedia force-pushed the feat/type-extension branch 2 times, most recently from 62b7c8b to 034c54c Compare May 20, 2025 12:44
@abemedia abemedia marked this pull request as ready for review May 20, 2025 14:20
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM in general, but encoding is tricky, so we need a bit more tests.

Types from aliased packages will contain the name of the alias in the derived types e.g. OptDecimal1Decimal. For unique package names they will work the same as other types e.g. OptDecimal. Happy to remove this functionality though and resort to forcing users to use x-ogen-name when they have clashes if this feels too strange.

Let's remove it, then.

@abemedia abemedia force-pushed the feat/type-extension branch from 728c62f to 2ecbdfd Compare June 4, 2025 11:58
@abemedia abemedia force-pushed the feat/type-extension branch from 2ecbdfd to f6fecc8 Compare June 4, 2025 12:05
@abemedia abemedia force-pushed the feat/type-extension branch 2 times, most recently from 8ece4b7 to f562a71 Compare June 4, 2025 12:25
@abemedia abemedia force-pushed the feat/type-extension branch 3 times, most recently from 53816aa to a51770d Compare June 4, 2025 13:09
@abemedia abemedia force-pushed the feat/type-extension branch 2 times, most recently from e454143 to dff62fb Compare June 4, 2025 13:18
@abemedia
Copy link
Contributor Author

abemedia commented Jun 4, 2025

@tdakkota I made a bunch of changes and improved the (un)marshalling of external values to avoid the reflection cost in the stdlib json package where possible. Could I get another review?
I also updated the gotchas in the PR message so I'd reread it to make sure you're happy with them.

@abemedia
Copy link
Contributor Author

abemedia commented Jun 5, 2025

Actually hold off I just found a bug. Will fix and report back when done.

@abemedia abemedia force-pushed the feat/type-extension branch 2 times, most recently from 5158f45 to c1fbc88 Compare June 6, 2025 23:29
@abemedia
Copy link
Contributor Author

abemedia commented Jun 6, 2025

@tdakkota I've resolved the bugs mentioned in the previous message and this is again ready for review.

@abemedia abemedia force-pushed the feat/type-extension branch from c1fbc88 to bb12d74 Compare June 7, 2025 19:50
@abemedia abemedia requested review from a user and utherbit June 7, 2025 19:50
@abemedia abemedia force-pushed the feat/type-extension branch 2 times, most recently from 70061c0 to a239b94 Compare June 23, 2025 12:14
@abemedia abemedia force-pushed the feat/type-extension branch from a239b94 to cc6435b Compare July 4, 2025 09:16
@abemedia
Copy link
Contributor Author

abemedia commented Jul 4, 2025

@tdakkota sorry for the delay but I have just added another commit to improve the encode/decode by removing the pointless string functions and added support for binary types.
Mind giving it another review? If you're happy I'll squash the commits, just thought this makes it easier to review the actual changes.

@abemedia abemedia force-pushed the feat/type-extension branch from 6f35464 to 51d5e76 Compare July 4, 2025 10:31
@abemedia abemedia force-pushed the feat/type-extension branch 3 times, most recently from abfb8ae to 6c991e5 Compare July 7, 2025 12:06
@abemedia abemedia force-pushed the feat/type-extension branch from 6c991e5 to 19fdbd3 Compare July 7, 2025 12:06
@abemedia
Copy link
Contributor Author

abemedia commented Jul 7, 2025

@tdakkota I've made the final small changes and squashed the comments so it's ready for merging unless you have any other concerns.

@abemedia abemedia requested a review from a user July 7, 2025 12:13
@ghost ghost enabled auto-merge July 10, 2025 01:52
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Well, it was a long road.

Huge thanks for all your contributions to the ogen.

@ghost ghost merged commit 72cb51b into ogen-go:main Jul 10, 2025
15 checks passed
@abemedia abemedia deleted the feat/type-extension branch July 10, 2025 07:51
@abemedia
Copy link
Contributor Author

Thank you!

This pull request was closed.
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.

Support custom format types.

2 participants