support for specifying version numbers in API endpoints #1115

davepacheco · 2024-09-24T19:40:35Z

Supersedes #862 (and is based on it). Fixes #869.

Also based on #1127. This includes a test that uses that option to make sure that operations have the same names in different versions, even when they have different handler function names.

…ersions

…d types in OpenAPI spec

…ersions

davepacheco · 2024-10-03T00:14:34Z

I think this one is nearly ready to go (but after #1127). The one change I'm leaning towards is that I think versions = A..B ought to not include B, for consistency with Rust's range syntax. I think people will be confused to discover that it actually means the thing that Rust denotes with A..=B.

…versions

davepacheco · 2024-10-07T22:05:37Z

@jgallagher and @leftwo raised the excellent point that earlier versions of this PR would cause a Dropshot server to happily serve requests for API versions newer than the server was built with. For now, we can make this the responsibility of the DynamicVersionPolicy impl. Dropshot ships with only one impl, ClientSpecifiesVersionInHeader. I've changed this one to require a max version and reject requests newer than that.

Stepping back: my goal for this work is to provide a minimal base for us to go prototype versioning in some real consumers. I think it's pretty likely that as we do that, we will find changes we want to make to this. I wouldn't be surprised if we wind up with some common impls of DynamicVersionPolicy that we want to fold back into Dropshot. But I don't want to spend too much energy now trying to guess what those things are -- let's see what shakes out of the consumer impls.

ahl

very nice

dropshot_endpoint/src/metadata.rs

ahl · 2024-10-18T19:20:39Z

dropshot/src/server.rs

+        let router = api.into_router();
+        if let VersionPolicy::Unversioned = version_policy {
+            if router.has_versioned_routes() {
+                return Err(BuildError::UnversionedServerHasVersionedRoutes);


is this the point of has_versioned_routes i.e. is it mostly here to produce an error message if the user has done something aberrant?

ahl · 2024-10-18T19:24:25Z

dropshot/tests/test_versions.rs

+        api.openapi("Evolving API", semver).write(&mut found).unwrap();
+        let actual = std::str::from_utf8(found.get_ref()).unwrap();
+        expectorate::assert_contents(
+            &format!("tests/test_openapi_v{}.json", version),


what do you think about

Suggested change

&format!("tests/test_openapi_v{}.json", version),

&format!("tests/test_versions_v{}.json", version),

or something to help associate json with rs?

ahl · 2024-10-18T19:27:45Z

dropshot_endpoint/src/metadata.rs

+                            "semver range (from ... until ...) has the \
+                             endpoints out of order",


from version is after until version?

Right. Are you suggesting changing the message? I'm fine with that (although I think it's a little hard to parse, using "from" and "until" as adjectives, especially if you're not already in the headspace of these arguments being called the "from version" and the "until version").

I found the suggestion that the numbers are out of order potentially misleading. e.g. if I have 1.2.3..1.2.2 and what I actually meant was 1.2.3..1.2.12 . I was trying to come up with some other way of expressing the problem to the user.

ahl · 2024-10-18T19:29:38Z

dropshot_endpoint/src/metadata.rs

+                    .into_iter()
+                    .collect();
+                    return Err(syn::Error::new_spanned(
+                        span,


I think we want

Suggested change

span,

input.span(),

Maybe? In particular I'm thinking of this test:

dropshot/tests/fail/bad_version_backwards.stderr

error: semver range (from ... until ...) has the endpoints out of order --> tests/fail/bad_version_backwards.rs:17:16 | 17 | versions = "1.2.3".."1.2.2" |

Should we be underlining both?

This doesn't quite work:

error[E0277]: the trait bound `proc_macro2::Span: ToTokens` is not satisfied --> dropshot_endpoint/src/metadata.rs:354:25 | 353 | return Err(syn::Error::new_spanned( | ----------------------- required by a bound introduced by this call 354 | input.span(), | ^^^^^^^^^^^^ the trait `ToTokens` is not implemented for `proc_macro2::Span` | = help: the following other types implement trait `ToTokens`: &'a T &'a mut T Abstract AndAnd AndEq AngleBracketedGenericArguments Arm As and 308 others note: required by a bound in `syn::Error::new_spanned` --> /Users/dap/.cargo/registry/src/index.crates.io-6f17d22bba15001f/syn-2.0.79/src/error.rs:189:27 | 189 | pub fn new_spanned<T: ToTokens, U: Display>(tokens: T, message: U) -> Self { | ^^^^^^^^ required by this bound in `Error::new_spanned` For more information about this error, try `rustc --explain E0277`. error: could not compile `dropshot_endpoint` (lib) due to 1 previous error warning: build failed, waiting for other jobs to finish...

I poked around a bit but did not immediately see how to turn that span into something that impls ToTokens (besides what the code is already doing).

ahl · 2024-10-18T19:31:18Z

dropshot_endpoint/src/metadata.rs

+    v.value()
+        .parse::<semver::Version>()
+        .map_err(|e| {
+            syn::Error::new_spanned(v, format!("expected semver: {}", e))


what do Semver::parse() errors look like? Are those more useful than something like "{} is not of the form MAJOR.MINOR.PATCH"?

I think the messages it produces are okay. Here are a few examples:

dropshot/dropshot_endpoint/src/endpoint.rs

Lines 1070 to 1085 in e747169

versions = "one dot two dot three",

},

quote! {

pub async fn handler_xyz(

_rqctx: RequestContext<()>,

) -> Result<HttpResponseOk<()>, HttpError> {

Ok(())

}

},

);

assert!(!errors.is_empty());

assert_eq!(

errors.get(0).map(ToString::to_string),

Some(

"expected semver: unexpected character 'o' while \

parsing major version number"

dropshot/dropshot_endpoint/src/endpoint.rs

Lines 1094 to 1109 in e747169

versions = "1.2",

},

quote! {

pub async fn handler_xyz(

_rqctx: RequestContext<()>,

) -> Result<HttpResponseOk<()>, HttpError> {

Ok(())

}

},

);

assert!(!errors.is_empty());

assert_eq!(

errors.get(0).map(ToString::to_string),

Some(

"expected semver: unexpected end of input while parsing \

minor version number"

ahl · 2024-10-28T22:00:19Z

Do we definitely want to allow users to specify the version number header (i.e. in ClientSpecifiesVersionInHeader)? Or is this flexibility they don't need? If we allow this to be configurable, we'll need a similar sort of configuration when generating clients.

We could document (in OpenAPI) this optional header for all API endpoints; that seems quite annoying. Alternatively we could invent an extension to tell progenitor if there's a version header and--if so--what to call it.

davepacheco · 2024-10-29T20:15:30Z

Do we definitely want to allow users to specify the version number header (i.e. in ClientSpecifiesVersionInHeader)? Or is this flexibility they don't need? If we allow this to be configurable, we'll need a similar sort of configuration when generating clients.

We could document (in OpenAPI) this optional header for all API endpoints; that seems quite annoying. Alternatively we could invent an extension to tell progenitor if there's a version header and--if so--what to call it.

We discussed this offline and concluded:

We'll pick a name that ought to be fine for all of our own stuff (probably api-version).
Progenitor will use this name by default, but the behavior will be configurable so that you can have it not specify anything or use a different header name.
Dropshot will make it easy to use the same header name. I'm being vague about the specifics here because I expect this part of the Dropshot interface to potentially change a bit when we start incorporating it into consumers so I plan to flesh this out when we do that.

davepacheco added 7 commits September 24, 2024 12:39

support for specifying version numbers in API endpoints

a5bfb88

update CHANGELOG

25c4646

fix up most clippy errors

4e5fcd1

add test for creating unversioned servers with versioned routes

e0d0003

add test for compiler error

ceaf0ad

add a real test for versioning

4cc1d1d

some refactoring; add a common impl; much documentation

8d4535e

This was referenced Sep 27, 2024

WIP: builder interface for constructing servers, rework server startup code paths #1120

Closed

add builder interface for making servers #1122

Merged

davepacheco added 2 commits October 2, 2024 13:31

allow overriding operation_id

115e74a

mention in the docs

d30fc78

davepacheco mentioned this pull request Oct 2, 2024

allow endpoints and channels to specify the operation_id #1127

Merged

davepacheco added 7 commits October 2, 2024 14:48

Merge branch 'main' into dap/api-versions

9d3755b

add tests for version-specific OpenAPI specs

1011374

Merge remote-tracking branch 'origin/dap/operation-id' into dap/api-v…

618f2c1

…ersions

add test that uses three different approaches to naming operations an…

67c135f

…d types in OpenAPI spec

fix test to actually test that types appear in the correct versions

6344e3c

fix whitespace

7a8683d

Merge remote-tracking branch 'origin/dap/operation-id' into dap/api-v…

c8810dc

…ersions

davepacheco changed the base branch from main to dap/operation-id October 2, 2024 23:44

davepacheco added 2 commits October 2, 2024 16:51

fix changelog mismerge

368c366

self-review nits

d19f3e0

davepacheco added 3 commits October 3, 2024 14:30

cleaner logging for endpoint versions

3d2addd

make the end of the versions range NON-inclusive

0f1bdac

update versioning example for clarity

2a73353

Base automatically changed from dap/operation-id to main October 7, 2024 16:56

davepacheco added 3 commits October 7, 2024 13:41

Merge commit '1c0a35335c35dab38703b31c8c0e80ed8dfb9f03' into dap/api-…

afc4494

…versions

Merge commit 'bde88ae32f190cccdd092c3bc959e600ea5116aa' into dap/api-…

9f6e066

…versions

ClientSpecifiesVersionInHeader should accept a max version

9a86f2a

davepacheco marked this pull request as ready for review October 7, 2024 21:42

davepacheco self-assigned this Oct 10, 2024

ahl reviewed Oct 18, 2024

View reviewed changes

review feedback (rename test output files)

e747169

Merge branch 'main' into dap/api-versions

58b1361

ahl approved these changes Oct 30, 2024

View reviewed changes

review feedback: update error message

2843d13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support for specifying version numbers in API endpoints #1115

support for specifying version numbers in API endpoints #1115

davepacheco commented Sep 24, 2024 •

edited

Loading

davepacheco commented Oct 3, 2024

davepacheco commented Oct 7, 2024

ahl left a comment

ahl Oct 18, 2024

davepacheco Oct 29, 2024

ahl Oct 18, 2024

davepacheco Oct 29, 2024

ahl Oct 18, 2024

davepacheco Oct 29, 2024

ahl Oct 29, 2024 •

edited

Loading

ahl Oct 18, 2024

davepacheco Oct 29, 2024

ahl Oct 18, 2024

davepacheco Oct 29, 2024

ahl commented Oct 28, 2024

davepacheco commented Oct 29, 2024

	&format!("tests/test_openapi_v{}.json", version),
	&format!("tests/test_versions_v{}.json", version),

		"semver range (from ... until ...) has the \
		endpoints out of order",

	versions = "one dot two dot three",
	},
	quote! {
	pub async fn handler_xyz(
	_rqctx: RequestContext<()>,
	) -> Result<HttpResponseOk<()>, HttpError> {
	Ok(())
	}
	},
	);
	assert!(!errors.is_empty());
	assert_eq!(
	errors.get(0).map(ToString::to_string),
	Some(
	"expected semver: unexpected character 'o' while \
	parsing major version number"

	versions = "1.2",
	},
	quote! {
	pub async fn handler_xyz(
	_rqctx: RequestContext<()>,
	) -> Result<HttpResponseOk<()>, HttpError> {
	Ok(())
	}
	},
	);
	assert!(!errors.is_empty());
	assert_eq!(
	errors.get(0).map(ToString::to_string),
	Some(
	"expected semver: unexpected end of input while parsing \
	minor version number"

support for specifying version numbers in API endpoints #1115

Are you sure you want to change the base?

support for specifying version numbers in API endpoints #1115

Conversation

davepacheco commented Sep 24, 2024 • edited Loading

davepacheco commented Oct 3, 2024

davepacheco commented Oct 7, 2024

ahl 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahl Oct 29, 2024 • 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

ahl commented Oct 28, 2024

davepacheco commented Oct 29, 2024

davepacheco commented Sep 24, 2024 •

edited

Loading

ahl Oct 29, 2024 •

edited

Loading