feat: implement online style fetching for stable style resolving #459

achou11 · 2024-01-31T00:01:16Z

Closes #439
Closes #448

Stacked on #450

Notes:

implements the online proxying component of determining which style.json to return handling the stable style url endpoint. Currently assumes that the client will provide an key query param when requesting the stable style json endpoint (as opposed to us using some hardcoded token)
e2e test(s) will be implemented once feat: add method to MapeoManager for getting stable map style json url #457 is merged

TODO:

~~Potentially use a proper fixture and/or making an actual request to the upstream provider? Right now I mock the response using Undici's mocking mechanisms~~ Added fixtures for style.json's from both Mapbox and Protomaps
Do we want to incorporate a compression plugin? (e.g. https://github.com/fastify/fastify-compress/). Most of the time, these upstream providers are compressing the responses but since we don't have anything set up, our forwarded response is not compressed right now EDIT: seems like we don't need to right now based on feedback

src/fastify-plugins/maps/index.js

achou11 · 2024-01-31T18:18:37Z

src/fastify-plugins/maps/index.js

+export const DEFAULT_MAPBOX_STYLE_URL =
+  'https://api.mapbox.com/styles/v1/mapbox/outdoors-v12'


not sure which style we'd like to use by default

I don't know, but I think it's probably fine to merge this as long as we eventually confirm this is reasonable.

src/fastify-plugins/maps/index.js

achou11 · 2024-01-31T18:21:57Z

src/fastify-plugins/maps/index.js

+            // Some upstream providers will not set the 'application/json' content-type header despite the body being JSON e.g. Protomaps
+            // TODO: Should we forward the upstream 'content-type' header?
+            // We kind of assume that a Style Spec-compatible JSON payload will always be used by a provider
+            // Tehcnically, there could be cases where a provider doesn't use the Mapbox Style Spec and has their own format,
+            // which may be delivered as some other content type
+            rep.header('content-type', 'application/json; charset=utf-8')
+            return upstreamResponse.json()


highlighting this. implementation currently assumes that most upstream providers will be working with a JSON-based style definition. technically, it could be some other format with a different content-type, but not sure how many providers like that actually exist.

the content-type header workaround is mostly defensive - just something i noticed when testing with Protomap's style API (which currently doesn't set an expected content-type: application/json header)

src/fastify-plugins/maps/index.js

tests/fastify-plugins/maps.js

EvanHahn

LGTM other than my numerous nitpicks and questions.

src/fastify-plugins/maps/index.js

test-e2e/manager-fastify-server.js

src/fastify-plugins/maps/index.js

tests/fastify-plugins/maps.js

src/fastify-plugins/maps/index.js

achou11 · 2024-01-31T22:50:13Z

src/fastify-plugins/maps/index.js

+            rep.header('content-type', 'application/json; charset=utf-8')
+            return upstreamResponse.json()
+          } else {
+            fastify.log.error(


@EvanHahn do you think this should this also be a warning instead of a error log?

Short answer: yes, I think it should be a warning.

Long answer:

At a high level, I believe two things about how errors should be logged:

Errors are for developer mistakes. Invalid states are a good example, because a perfect developer would never put the system into an invalid state.

Warnings are for unavoidable problems. Network failures are usually a good example, because even a perfect developer can't fix a user turning their wifi off.

To be clear, this is personal preference!

So what would I do this case?

If we get a 5xx error from the server, that's a warning, because we didn't make a mistake. But you could argue that we should error if we get a 4xx. After all, if we send a bogus request, isn't that a developer's fault?

In this case, I think the answer is...maybe? Do we consider a bad API key an error, or a warning? Does it depend on what kind of 4xx status code you get (e.g., is 422 "our fault" but 429 is not)? I don't think it's worth thinking too hard about, so I'd probably just go with a warning.

this is helpful! thanks for sharing both answers 😄

addressed via 0d992ef

EvanHahn · 2024-02-01T23:23:17Z

I plan to review this on Monday.

EvanHahn

LGTM as long as we get an answer to this question (doesn't have to be before merge):

https://github.com/digidem/mapeo-core-next/pull/459/files#r1473479880

src/fastify-plugins/maps/index.js

EvanHahn · 2024-02-09T13:01:01Z

Not planning to re-review this. Let me know if that's wrong!

EvanHahn · 2024-02-20T14:08:23Z

@achou11 Anything else you want to do on this PR before we merge it?

achou11 · 2024-02-20T14:09:52Z

@achou11 Anything else you want to do on this PR before we merge it?

considering it's stacked, I'd prefer to merge the other ones first based on stack order. the other ones haven't been reviewed yet and so id refrain from merging in order to make them easier to review

EvanHahn · 2024-02-20T15:41:26Z

Ah, of course. Sorry for the ping!

achou11 requested review from gmaclennan and EvanHahn January 31, 2024 00:01

achou11 mentioned this pull request Jan 31, 2024

Implement style.json resolution strategy a when handling the stable style url #2 #448

Closed

achou11 force-pushed the ac/fetch-online-style branch from 667e688 to 30b96a5 Compare January 31, 2024 20:57

achou11 commented Jan 31, 2024

View reviewed changes

EvanHahn approved these changes Jan 31, 2024

View reviewed changes

achou11 commented Jan 31, 2024

View reviewed changes

achou11 requested a review from EvanHahn January 31, 2024 23:00

achou11 force-pushed the ac/offline-fallback-map branch from d6f96b6 to 096e0d9 Compare February 1, 2024 16:10

achou11 force-pushed the ac/fetch-online-style branch from 0654550 to a6d8c44 Compare February 1, 2024 16:10

EvanHahn approved these changes Feb 5, 2024

View reviewed changes

src/fastify-plugins/maps/index.js Outdated Show resolved Hide resolved

achou11 force-pushed the ac/offline-fallback-map branch from 096e0d9 to 3b6d2ce Compare February 5, 2024 16:52

achou11 force-pushed the ac/fetch-online-style branch from a6d8c44 to 0d992ef Compare February 5, 2024 16:52

EvanHahn approved these changes Feb 5, 2024

View reviewed changes

achou11 force-pushed the ac/offline-fallback-map branch from 3b6d2ce to e37954e Compare February 8, 2024 21:37

achou11 force-pushed the ac/fetch-online-style branch from ef1c0dd to dff966d Compare February 8, 2024 21:37

achou11 force-pushed the ac/offline-fallback-map branch from e37954e to 8153ae9 Compare February 29, 2024 20:44

achou11 force-pushed the ac/fetch-online-style branch from cc0c3ff to 9ed4430 Compare February 29, 2024 20:45

achou11 mentioned this pull request Mar 6, 2024

chore: update Undici to latest version #509

Merged

achou11 force-pushed the ac/offline-fallback-map branch from 8153ae9 to ef363dd Compare March 11, 2024 16:51

achou11 force-pushed the ac/fetch-online-style branch from 9ed4430 to 2abe8a4 Compare March 11, 2024 16:51

achou11 force-pushed the ac/offline-fallback-map branch from ef363dd to af62ed3 Compare March 12, 2024 18:08

achou11 force-pushed the ac/fetch-online-style branch from 2abe8a4 to 0a84122 Compare March 12, 2024 18:08

achou11 force-pushed the ac/offline-fallback-map branch from af62ed3 to be88383 Compare March 12, 2024 18:15

Base automatically changed from ac/offline-fallback-map to main March 12, 2024 18:20

achou11 added 3 commits March 12, 2024 14:22

change error log to warning

201788e

switch to allow-list when forwarding headers

eeced70

fix lockfile

12ea285

achou11 force-pushed the ac/fetch-online-style branch from 0a84122 to 12ea285 Compare March 12, 2024 18:23

achou11 merged commit 4ca6d7c into main Mar 12, 2024
4 checks passed

achou11 deleted the ac/fetch-online-style branch March 12, 2024 18:37

optic-release-automation bot mentioned this pull request Mar 14, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.5 #519

Merged

This was referenced Apr 11, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.6 #558

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.7 #573

Merged

optic-release-automation bot mentioned this pull request Apr 30, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.8 #592

Merged

optic-release-automation bot mentioned this pull request May 9, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.9 #632

Merged

This was referenced Jun 20, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.10 #709

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.11 #711

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.12 #714

Merged

This was referenced Jul 25, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.13 #726

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.14 #740

Merged

optic-release-automation bot mentioned this pull request Aug 13, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.15 #748

Merged

This was referenced Aug 21, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.16 #758

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.17 #787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement online style fetching for stable style resolving #459

feat: implement online style fetching for stable style resolving #459

achou11 commented Jan 31, 2024 •

edited

Loading

achou11 Jan 31, 2024

EvanHahn Jan 31, 2024

achou11 Jan 31, 2024

EvanHahn left a comment •

edited

Loading

achou11 Jan 31, 2024

EvanHahn Feb 5, 2024

achou11 Feb 5, 2024

achou11 Feb 5, 2024

EvanHahn commented Feb 1, 2024 via email

EvanHahn left a comment

EvanHahn commented Feb 9, 2024

EvanHahn commented Feb 20, 2024

achou11 commented Feb 20, 2024 •

edited

Loading

EvanHahn commented Feb 20, 2024 via email

		export const DEFAULT_MAPBOX_STYLE_URL =
		'https://api.mapbox.com/styles/v1/mapbox/outdoors-v12'

feat: implement online style fetching for stable style resolving #459

feat: implement online style fetching for stable style resolving #459

Conversation

achou11 commented Jan 31, 2024 • edited Loading

achou11 Jan 31, 2024

Choose a reason for hiding this comment

EvanHahn Jan 31, 2024

Choose a reason for hiding this comment

achou11 Jan 31, 2024

Choose a reason for hiding this comment

EvanHahn left a comment • edited Loading

Choose a reason for hiding this comment

achou11 Jan 31, 2024

Choose a reason for hiding this comment

EvanHahn Feb 5, 2024

Choose a reason for hiding this comment

achou11 Feb 5, 2024

Choose a reason for hiding this comment

achou11 Feb 5, 2024

Choose a reason for hiding this comment

EvanHahn commented Feb 1, 2024 via email

EvanHahn left a comment

Choose a reason for hiding this comment

EvanHahn commented Feb 9, 2024

EvanHahn commented Feb 20, 2024

achou11 commented Feb 20, 2024 • edited Loading

EvanHahn commented Feb 20, 2024 via email

achou11 commented Jan 31, 2024 •

edited

Loading

EvanHahn left a comment •

edited

Loading

achou11 commented Feb 20, 2024 •

edited

Loading