feat: Restructure AIPs 213 and 215, with more common proto details #1117

jskeet · 2023-05-23T10:28:06Z

This is a second attempt, which keeps API-specific protos separate from common protos.

jskeet · 2023-05-24T15:53:39Z

This is an alternative to #1107.

noahdietz

Love it!

toumorokoshi

Overall LGTM. The only blocker I have is the should on being aware - doesn't feel actionable.

toumorokoshi · 2023-05-28T05:03:06Z

aip/general/0213.md

+  organization-wide common proto package.
+- Organizations **must not** define generic protos in organization-specific
+  common protos, instead preferring global common protos.
+- Common protos **must not** be "moved" from an organization-specific common


what if what was originally thought to be an organization-specific concept ends up being more globally applicable? e.g. other organizations would like to use it. Would each organization duplicate the original organization-specific proto?

Either each org could duplicate it, or we could add it to the global common protos, but with the original organization having to stick with their own copy - at least for existing fields. (Perhaps use the new global proto for new APIs, but stick with the org-specific one for all existing APIs using it? Both options are a bit messy.)

Given that this isn't a great experience either way, we should try to avoid getting into this mess - suggestions for processes to help on that front would be welcome!

If the type "graduates" to google.type, and remains wire-compatible (serialized proto and JSON), what's the problem with switching to the new copy of the type in google.type?

It's a breaking change for API libraries in statically-typed languages, because the type would be different. Removing it from the org-specific common library would also be a breaking change, as removing public types always is.

The latter breaking change is the big problem, as a new major version of the org-specific common library requires a new major version of all API libraries depending on it.

Basically there are ways this could just about be done, but it's a lot of work - much better to really take the time to work out global/org-specific first. (The DateRange one earlier is a good example of a gray area... we may have got that wrong, but it's unclear. It's not obviously org-specific in that date ranges themselves are quite general - but the detailed requirements of the type around precision, inclusivity etc are org-specific.)

If I have an API client that expects google.shoe.type.ShoeSize and the server is updated to use a graduated google.type.ShoeSize...why would my client break?

I agree that a generated GAPIC in a statically typed language would contain a breaking change when regenerated after the switch to the graduated type, but that seems distinct from a breaking change in the API itself. Client code that isn't updated at all would keep working, right?

I agree that a generated GAPIC in a statically typed language would contain a breaking change when regenerated after the switch to the graduated type

And that's what I'm talking about.

that seems distinct from a breaking change in the API itself.

We've always included "that would cause a breaking change in client libraries" as a breaking change, and I'd want to keep doing so. AIP-180 explicitly calls that out with 3 kinds of compatibility - this would be compatible with item 2 and 3 there, but not with item 1.

Client code that isn't updated at all would keep working, right?

Yes, unless the type is used as an "Any". But there's more to compatibility than "existing code against existing libraries keeps working".

Yeah fair enough - since AIP-180's item 1 is explicitly includes newer versions of client libraries, that's that I suppose.

(Not resolving this thread since I didn't start it, but FWIW, it's resolved as far as I'm concerned.)

@toumorokoshi: Are you happy to resolve this now?

aip/general/0213.md

toumorokoshi · 2023-05-28T05:25:03Z

FYI also to @rofrankel who's been looking into common protos for more general AIPs.

aip/general/0215.md

rofrankel · 2023-05-31T00:44:03Z

aip/general/0213.md

+- Organizations **must not** define generic protos in organization-specific
+  common protos, instead preferring global common protos.


...must not define...protos...in...protos

Something about this wording feels off. Maybe "organization-specific packages" rather than "organization-specific protos"?

Will go over that at the same time as looking at protos/components.

aip/general/0213.md

rofrankel · 2023-05-31T00:48:01Z

aip/general/0213.md

+  organization-wide common proto package.
+- Organizations **must not** define generic protos in organization-specific
+  common protos, instead preferring global common protos.
+- Common protos **must not** be "moved" from an organization-specific common


If the type "graduates" to google.type, and remains wire-compatible (serialized proto and JSON), what's the problem with switching to the new copy of the type in google.type?

aip/general/0213.md

jskeet

@rofrankel: I've taken another pass at this - PTAL. AIP-215 is still more "proto-oriented" than "component-oriented" as it's describing the API protos themselves; I'm hoping that's okay.

Feedback on any of the changes is welcome.

aip/general/0213.md

rofrankel · 2023-06-02T17:08:04Z

aip/general/0213.md

+  organization-wide common proto package.
+- Organizations **must not** define generic protos in organization-specific
+  common protos, instead preferring global common protos.
+- Common protos **must not** be "moved" from an organization-specific common


(Not resolving this thread since I didn't start it, but FWIW, it's resolved as far as I'm concerned.)

rofrankel

Approving in order not to block any longer on the one remaining open thread - any resolution to it works for me. In general this seems like a good PR and it should get merged.

jskeet · 2023-06-07T16:27:15Z

Added "do not merge" so that client libraries folks can take a look before we merge.

aip/general/0213.md

jskeet · 2023-06-15T07:27:24Z

@toumorokoshi: Please could you reapprove? (You requested changes about "should be aware" - I took your suggestion.)

dazuma · 2023-06-15T16:45:07Z

aip/general/0215.md

+  resource names ([AIP-122][]), rather than using the resource messages.
+- When two versions of an API use effectively the same (API-specific) proto
+  that proto **must** be duplicated in each version. (In other words, APIs
+  **must not** create their own "API-specific common comonent" packages.)


s/comonent/component/

Darn, again? Sorry, will fix. (I'm going to look at the more significant comment tomorrow.)

jskeet · 2023-06-20T14:44:53Z

@toumorokoshi: Ping for reapproval? Will squash and merge if so - I think everyone from Client Libraries is happy now (or at least has had ample chance to raise objections).

bshaffer · 2023-06-20T16:50:56Z

aip/general/0213.md

+"generic" and "organization-specific" is a gray area; some generic *concepts*
+have organization-specific use cases which surface through the components.
+
+## Existing global common components


I'd love to see a section for ## Existing organization-wide common components as well. I understand that this runs the risk of going out of date, but it would still be very helpful to categorize the ones that exist today for reference.

Okay - I'm basically on vacation now until next week, and I'd like to get this merged before then if possible, but I can definitely take this as an action item for next week.

SGTM! Added an issue to follow up #1142

toumorokoshi · 2023-06-22T14:03:25Z

aip/general/0213.md

+  organization-wide common component package.
+- Organizations **must not** define generic components in organization-specific
+  common component packages, instead preferring global common components.
+- Common components **must not** be "moved" from an organization-specific common


I think it'd be nice to define "move" a bit more here - is completely copying the proto not allowed? if not, what happens when a concept does become common across multiple protos?

Added clarification, and a "may" for copy-without-removal.

aip/general/0213.md

toumorokoshi

A couple nits but nothing blocking. Thanks!

jskeet

Addressed review comments in a new commit - @toumorokoshi if you could review just the last commit to check that it works for you, that would be great.

aip/general/0213.md

jskeet · 2023-06-26T14:56:35Z

aip/general/0213.md

+  organization-wide common component package.
+- Organizations **must not** define generic components in organization-specific
+  common component packages, instead preferring global common components.
+- Common components **must not** be "moved" from an organization-specific common


Added clarification, and a "may" for copy-without-removal.

This is a second attempt, which keeps API-specific protos separate from common protos.

Co-authored-by: Brent Shaffer <[email protected]>

jskeet requested a review from a team as a code owner May 23, 2023 10:28

shwoodard mentioned this pull request May 24, 2023

feat: Combine AIPs 213 and 215, with more common proto details #1107

Closed

noahdietz approved these changes May 26, 2023

View reviewed changes

toumorokoshi suggested changes May 28, 2023

View reviewed changes

rofrankel requested changes May 31, 2023

View reviewed changes

jskeet force-pushed the common-protos-2 branch 2 times, most recently from e7e5248 to fc6fc33 Compare June 2, 2023 13:59

jskeet commented Jun 2, 2023

View reviewed changes

rofrankel reviewed Jun 2, 2023

View reviewed changes

rofrankel approved these changes Jun 6, 2023

View reviewed changes

jskeet added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 7, 2023

alexander-fenster reviewed Jun 7, 2023

View reviewed changes

aip/general/0213.md Outdated Show resolved Hide resolved

alexander-fenster approved these changes Jun 7, 2023

View reviewed changes

jskeet force-pushed the common-protos-2 branch from a03b099 to da0b5f4 Compare June 15, 2023 07:27

dazuma reviewed Jun 15, 2023

View reviewed changes

jskeet force-pushed the common-protos-2 branch from da0b5f4 to 5bf4949 Compare June 16, 2023 08:19

jskeet removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 20, 2023

bshaffer reviewed Jun 20, 2023

View reviewed changes

bshaffer mentioned this pull request Jun 20, 2023

AIP-213: add a section for existing org-specific common protos #1142

Closed

toumorokoshi reviewed Jun 22, 2023

View reviewed changes

aip/general/0213.md Show resolved Hide resolved

toumorokoshi approved these changes Jun 22, 2023

View reviewed changes

jskeet commented Jun 26, 2023

View reviewed changes

jskeet force-pushed the common-protos-2 branch from 5bf4949 to f696aac Compare June 26, 2023 14:57

feat: Restructure AIPs 213 and 215, with more common proto details

4f45085

This is a second attempt, which keeps API-specific protos separate from common protos.

jskeet and others added 7 commits June 27, 2023 16:30

Attempt to address review comments

70d556f

Make change log entries full sentences

529ab32

Add example of generic-sounding org-specific component

2929e85

Update aip/general/0213.md

cc3ffa4

Co-authored-by: Brent Shaffer <[email protected]>

Make propagation time more actionable

16d368b

Address review comments

42ddb77

Address review comments

9a490ee

jskeet force-pushed the common-protos-2 branch from f696aac to 9a490ee Compare June 27, 2023 15:30

jskeet enabled auto-merge (squash) June 27, 2023 15:31

jskeet merged commit 7a3a67a into aip-dev:master Jun 27, 2023

jskeet deleted the common-protos-2 branch June 27, 2023 15:31

jskeet mentioned this pull request Jun 28, 2023

AIP-213: Include more detailed rationale #1104

Open

noahdietz mentioned this pull request Feb 4, 2025

AIP-215: warning on cross-API references googleapis/api-linter#1018

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Restructure AIPs 213 and 215, with more common proto details #1117

feat: Restructure AIPs 213 and 215, with more common proto details #1117

jskeet commented May 23, 2023

jskeet commented May 24, 2023

noahdietz left a comment

toumorokoshi left a comment

toumorokoshi May 28, 2023

jskeet May 30, 2023

rofrankel May 31, 2023

jskeet May 31, 2023

rofrankel Jun 1, 2023 •

edited

Loading

jskeet Jun 1, 2023

rofrankel Jun 1, 2023

rofrankel Jun 2, 2023

jskeet Jun 15, 2023

toumorokoshi commented May 28, 2023

rofrankel May 31, 2023

jskeet May 31, 2023

rofrankel May 31, 2023

jskeet left a comment

rofrankel Jun 2, 2023

rofrankel left a comment

jskeet commented Jun 7, 2023

jskeet commented Jun 15, 2023

dazuma Jun 15, 2023

jskeet Jun 15, 2023

jskeet Jun 16, 2023

jskeet commented Jun 20, 2023

bshaffer Jun 20, 2023 •

edited

Loading

jskeet Jun 20, 2023

bshaffer Jun 20, 2023

toumorokoshi Jun 22, 2023

jskeet Jun 26, 2023

toumorokoshi left a comment

jskeet left a comment

jskeet Jun 26, 2023

		- Organizations must not define generic protos in organization-specific
		common protos, instead preferring global common protos.

feat: Restructure AIPs 213 and 215, with more common proto details #1117

feat: Restructure AIPs 213 and 215, with more common proto details #1117

Conversation

jskeet commented May 23, 2023

jskeet commented May 24, 2023

noahdietz left a comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rofrankel Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toumorokoshi commented May 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rofrankel left a comment

Choose a reason for hiding this comment

jskeet commented Jun 7, 2023

jskeet commented Jun 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet commented Jun 20, 2023

bshaffer Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rofrankel Jun 1, 2023 •

edited

Loading

bshaffer Jun 20, 2023 •

edited

Loading