Add support for adding base-route for Runtime#1506
Merged
Conversation
Removed some tests that weren't testing the CLI but dependencies (such as CommandLineParser or ILogger)
…them simpler and clearer with the new object structure
Moved logic for Entities defaults to RuntimeEntities rather than deserialiser, as that is a more logical place. We always pass that type around, so we can assume the ctor ran, but we aren't always assuming the deserialiser ran (such as what happens with tests)
Moving to a model where we load the config initially from disk and then generate an in-memory provider using an edited version of the config for the tests
Had introduced a regression that meant custom file names weren't supported on the engine directly. Now that can be passed to the RuntimeConfigLoader, otherwise the default filename is used. This means we're more flexible on the name of the file (useful in tests)
Singular/Plural were collapsed one level early, needed to nest them on a Type property
- Removing shared state as it can cause conflicts - Better modeling of a config for the tests, and swapping in mock services - Fixing the lookup for entities with the @model directive
…asn't the expected result
Had forgotten to remove some old code in the deserializer of RuntimeEntities as well
Aniruddh25
reviewed
Jul 12, 2023
Collaborator
Aniruddh25
left a comment
Aniruddh25
left a comment
There was a problem hiding this comment.
Extended functionality of Utils.IsApiPathValid(string? apiPath, ApiType apiType) method to Utils.IsURIComponentValid(string? uriComponent, ApiType apiType, string apiProperty) because similar validations are required for api properties: rest.path/ rest.base-route/ graphql.path, all of which are part of the request URI.
This description needs to be fixed.
Aniruddh25
reviewed
Jul 12, 2023
Contributor
Author
|
As per discussion with Ani over call:
|
seantleonard
approved these changes
Jul 14, 2023
Contributor
seantleonard
left a comment
seantleonard
left a comment
There was a problem hiding this comment.
Just a few suggestions but otherwise thanks for addressing the non-swa scenario validation and adding tests!
Contributor
seantleonard
left a comment
seantleonard
left a comment
There was a problem hiding this comment.
Feedback on latest updates.
Aniruddh25
reviewed
Jul 17, 2023
Aniruddh25
reviewed
Jul 17, 2023
Aniruddh25
approved these changes
Jul 17, 2023
seantleonard
approved these changes
Jul 17, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why make this change?
SWA or any other front end can interact with DAB using a path-based routing mechanism, where they may use a path (
base-route) to redirect requests to DAB (refer to the bug encountered: #1420). Thisbase-routegets stripped off when the request lands at DAB. Consequently, DAB is not even aware of anything like abase-routebeing used by the front end.This poses a problem in generation of the
nextLink, where thenextLinkwhich DAB generates, does not contain thebase-routeand consequently when the front end receives the next request, it does not find the base route in the request URL and hence, has no idea where to redirect the request to.Using the newly added option to configure base route for rest requests via the config (CLI option -
--runtime.base-route), the front-end like SWA can make DAB aware that they are using a base route to redirect requests to DAB. DAB can then accordingly account for the base route while generating thenextLinkfor paginated responses.What is this change?
base-routein global runtime settings in the config which can be via CLI as well using--runtime.base-routeoption in thedab initcommand. By default, it will be initialized as null. When the base-route is a non-empty string, it will be included in the generated nextLink. This feature is only supported whenauthentication provider == StaticWebApps. We throw an exception when base-route is configured for non-SWA providers.Utils.IsApiPathValid(string? apiPath, ApiType apiType)method toUtils.IsUriComponentValid(string? uriComponent)because similar validations are required for api properties:rest.path/runtime.base-route/graphql.path, all of which are part of the request URI.RuntimeConfigValidator.ValidateRestPathForRelationalDbs()toRuntimeConfigValidator.ValidateRestURI().RuntimeConfigValidator.ValidateGraphQLPath()toRuntimeConfigValidator.ValidateGraphQLURI()so that we follow same convention between rest and gql.Q&A
No, the existing config file will work perfectly fine. The base-route is totally an optional feature.
The default value for the base-route is null. This essentially means that base-route will not come into the picture.
The default value will be same irrespective of dev/prod/swa/non-swa environment and equal to null.
So to say, the change is not a breaking change and we are totally backwards compatible.
How was this tested?
ConfigurationTests.csto assert that the generated nextLink contains the base-route.ConfigValidationUnitTests.csto confirm that we throw proper exceptions when runtime base-route is formatted incorrectly (contains reserved characters or doesn't start with a '/') or when base-route is configured for non-SWA authentication providers.