Rewrite/Redesign Photon Go spec #187

steebchen · 2019-09-27T16:19:19Z

This PR introduces a complete rewrite of the Photon Go spec.

Rendered view
Rendered diff

TODO:

update links to the new spec in the docs

matthewmueller

Looks good! This can definitely help us form a basis for discussion tomorrow 👏

What I like:

Fluent API on columns (CreatedAt.Lt(...))
Better type-safety on inputs
New idea with PQL

What I don't like:

Exec(...), it bothers me that an edge case leaks into the entire API.
user.Name.Valid my preference is still pointers. I don't think this adds safety or clarity, it just adds extra methods
photon.Comment.CreatedAt.ASC, I'd still like to split this into separate comment.CreatedAt.ASC.

matthewmueller · 2019-09-30T14:28:59Z

photongo/Readme.md

+  CreatedAt time.Time  `json:"createdAt"`
+  UpdatedAt time.Time  `json:"updatedAt"`
+  Title     string     `json:"title"`
+  Desc      NullString `json:"desc"`


Can you provide a full working snippet? It's hard to know where these types are coming from.

matthewmueller · 2019-09-30T14:31:37Z

photongo/Readme.md

+  photon.User.ID.Set("7d42"),
+  photon.User.Email.Set("bob@prisma.io"),
+  photon.User.Name.Set("Bob"),
+).CreatePost(


The above example creates multiple posts. How would I create multiple posts in your proposal?

Good point, that's currently missing. I'll add it soon

matthewmueller · 2019-09-30T14:32:37Z

photongo/Readme.md

-  }
-}
-
-err := client.User.Select(&u,


Do you have a proposal for Select?

If you just want to select multiple things, you can query that by using the .With() methods.

matthewmueller · 2019-09-30T14:34:21Z

photongo/Readme.md

+  map[string]interface{}{
+    "id": "alice@prisma.io",
+  },
+)


I think we probably won't want to expose our internal protocol since it will change, but it's an interesting idea once we decide to stabilize it. Most likely it'll be a JSON-based protocol.

Ah okay, interesting. I thought maybe it would be cool for advanced usage, but in theory everything which exists in PQL would also be quickly available in photon clients. So yeah, we should probably not expose this

matthewmueller · 2019-09-30T14:36:00Z

photongo/Readme.md

+err := client.User.Raw(
+  ctx,
+  &u,
+  `SELECT * FROM users WHERE email = ?`,


Not a huge fan of the question marks since they're easy to mess up. Also MySQL specific. Have you seen this?

https://github.com/prisma/specs/blob/raw-spec/photon-raw-api/Readme.md#photon-go

Do you like this syntax? Any adjustments you'd like to make?

Fully agree here, thanks for the raw spec link. I just added a small example to have something.

matthewmueller · 2019-10-01T11:46:17Z

photongo/Readme.md

+  photon.User.Email.Equals("alice@prisma.io"),
+).Data(
+  photon.User.Email.Set("alice@prisma.io"),
+  photon.User.Role.Set(user.Role.ADMIN),


If we go with the multi-package route, could we do user.Role(...)?

I thought about this more last night and even though it's a really cool idea, I think it'll be too verbose in practice. We have to remember the context of where this code lives and who we're catering for. Even if we made photon incredibly type-safe in Go, the rest of their application logic won't be. Personally, I'd rather work with a succinct API and cater to professionals who write tests where these tests will catch the occasional runtime error.

Another tidbit: we can't ever make row-level violations type-safe (unique constraints). We're only tackling a subset of possible runtime errors with this API change.

Sorry I should have clarified on what's too verbose:

// upsert the user by their slack ID u := user.New(). SlackID(slackUser.SlackID). Username(slackUser.Username). Email(slackUser.Email). FirstName(slackUser.FirstName). LastName(slackUser.LastName). AvatarURL(slackUser.AvatarURL). Timezone(slackUser.Timezone). IsTeamOwner(true). TeamID(tm.ID) usr, err := user.UpsertBySlackIDAndTeamID(tx, res.UserID, tm.ID, u) if err != nil { return errors.Wrap(err, "unable to upsert the user") }

This is where I'm coming from, let's maybe reformat this in the new syntax and we can discuss further 🙏

I'm pulling the dot syntax argument here again.. The only char difference you have is .Set, which is negligible.

And, more importantly, my solution enforces the user to specify required fields with fixed parameters.

// generated code func (UserChain) Data(id UserIDParam, username UserUsernameParam /* ... */) { // ... }

so you can do

user, err := client.User.Upsert.Where( photon.User.Email.Equals("alice@prisma.io"), ).Data( photon.User.ID.Set("alice@prisma.io"), // .ID returns UserIDParam, so you are forced to sepcify required fields photon.User.Email.Set("alice@prisma.io"), // ... )

I'd love to see examples of the exact syntactic differences. I think it's more than .Set. Here's my rough translation:

u, err := user.UpsertBySlackIDAndTeamID(tx, res.UserID, tm.ID, user.New(). SlackID(slackUser.SlackID). Username(slackUser.Username). Email(slackUser.Email). FirstName(slackUser.FirstName). LastName(slackUser.LastName). AvatarURL(slackUser.AvatarURL). Timezone(slackUser.Timezone). IsTeamOwner(true). TeamID(tm.ID), ) // vs user, err := client.User.Upsert.Where( photon.User.SlackID.Equals("alice@prisma.io"), photon.User.TeamID.Equals("alice@prisma.io"), ).Data( photon.User.SlackID.Set(slackUser.SlackID), photon.User.Username.Set(slackUser.Username), photon.User.Email.Set(slackUser.Email), photon.User.FirstName.Set(slackUser.FirstName), photon.User.LastName.Set(slackUser.LastName), photon.User.AvatarURL.Set(slackUser.AvatarURL), photon.User.Timezone.Set(slackUser.Timezone), photon.User.IsTeamOwner.Set(true), photon.User.TeamID.Set(tm.ID), )

Please correct if wrong 🙏

^ disregard UpsertBySlackIDAndTeamID , it's more like user.Where().SlackID().ID()

It's much better with dot imports:

import ( . "github.com/user/repo/photon" ) // ... user, err := client.User.Upsert( User.SlackID.Set(u.SlackID), User.Email.Set(u.Email), User.TeamID.Equals(u.TeamID), User.SlackID.Set(u.SlackID), User.Username.Set(u.Username), User.FirstName.Set(u.FirstName), User.LastName.Set(u.LastName), User.AvatarURL.Set(u.AvatarURL), User.Timezone.Set(u.Timezone), User.IsTeamOwner.Set(true), User.TeamID.Set(tm.ID), )

I agree that this is still more verbose than yours, but I think it's worth to have the type safety here. This is the only reliable way to verify that all required fields are in fact specified. If you change or add a new required field in your data model, you will get warnings or errors for migrations if it would involve a breaking change. I would like to reflect this in the Go client as well, so you are forced to also adapt your code. I bet you that people (myself included) will forget to adapt their code for some .Create or .Upsert calls, Go would default to "" and users will wonder why their data is inconsistent or their queries won't work afterwards.

I think Go is more about simplicity and type safety than verbosity. Of course, it tries to be terse, but not when you have additional type safety – that's why you have verbose error checks because it may be easier to debug later, no generics which forces you to write specific functions for specific data types which means you have extremely high type safety.

Also, another advantage of using methods on each field, is perfect for nullable fields, regardless of queries (.Equals etc.) or setters. For example, if you have a nullable field, we may add .SetOptional/.Opt/whatever to nullable fields which accepts a string instead of a NullString/pointer if you want to set an optional value to a fixed string so there's no need a need for extra helper methods.

If we want to most simple method, I think that would be using structs, especially because IDEs can fill all fields with a single click, and you can even omit the field names. This would have the equal amount of type safety as your proposal, but the same drawbacks (not forced to specify required fields).

Related: steebchen/prisma-client-go#8

steebchen added 4 commits September 27, 2019 18:17

feat(photon-go): refactor photon spec

9a87abe

feat(photon-go): add generated go types and general examples

8449f10

chore(photon-go): update toc

b8849d2

fix(photon-go): minor fixes

fbba366

steebchen force-pushed the feat/photon-go branch from 80fad22 to fbba366 Compare September 30, 2019 10:31

github-actions bot and others added 2 commits September 30, 2019 10:31

docs: Update TOC

3a35b6c

fix(photon-go): minor fixes

298c49d

matthewmueller reviewed Sep 30, 2019

View reviewed changes

steebchen and others added 2 commits October 1, 2019 12:42

feat(photon-go): add basic aggregation API

b2ca6b1

docs: Update TOC

12552c7

matthewmueller reviewed Oct 1, 2019

View reviewed changes

steebchen and others added 2 commits October 2, 2019 13:20

fix(photon-go): minor fixes

2c3f5f7

docs: Update TOC

cea5001

janpio added the area/prisma-client-go label Oct 2, 2019

steebchen and others added 2 commits October 7, 2019 18:33

feat(photon-go): add advanced aggregations and orderBy

6cac2fb

docs: Update TOC

d519fb9

janpio changed the title ~~Refactor Photon Go spec~~ Rewrite/Redesign Photon Go spec Oct 9, 2019

janpio added kind/spec Something about an actual spec (file) spec/change An existing spec should be changed or adapted labels Oct 9, 2019

steebchen and others added 11 commits October 14, 2019 14:58

feat(photon-go): adapt methods to use singular/plural name

0281aa3

remove explicit where

97f07f7

use methods for nullability

91b9904

add relations to advanced queries

0e43d96

docs: Update TOC

1aa0a53

add simple update operations

c5a199d

fix

3f8c755

improve nested creates

bf065c5

docs: Update TOC

7ea4435

indent example headings by one

e2120f1

chore(photon-go): use single lines

045cfbd

steebchen and others added 3 commits October 15, 2019 17:51

feat(photon-go): refine paragraphs

584f57d

docs: Update TOC

2a156d8

chore(photon-go): mark code block as prisma schema language

389cd6e

Rewrite/Redesign Photon Go spec #187

Are you sure you want to change the base?

Rewrite/Redesign Photon Go spec #187

Uh oh!

Conversation

steebchen commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthewmueller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewmueller Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewmueller Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steebchen Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

steebchen commented Sep 27, 2019 •

edited

Loading

matthewmueller Oct 1, 2019 •

edited

Loading

matthewmueller Oct 2, 2019 •

edited

Loading

steebchen Oct 2, 2019 •

edited

Loading