Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor js_generic to use common DynamicJSEndpointRegistry hierarchy, and expose as an extensible public header #6710

Merged
merged 11 commits into from
Dec 19, 2024

Conversation

eddyashton
Copy link
Member

@eddyashton eddyashton commented Dec 18, 2024

Resolves #6682.

We copied a bunch of functionality from js_generic to DynamicJSEndpointRegistry, so that the latter could be extended by apps to run their own JS endpoints. Eventually we wanted near-identical functionality, so these implementations have a lot of duplication. This merges the 2, deleting the standalone js_generic code and making it instead extend the DynamicJSEndpointRegistry hierarchy. By using its own (not-quite-just-a-common-prefix) table names, and pushing some missing functionality up the stack into DynamicJSEndpointRegistry, we get to delete a load of code and get back to functional parity.

The commits in this PR show slightly cleaner steps than the final diff, but the key changes are:

  • DynamicJSEndpointRegistry splits out a BaseDynamicJSEndpointRegistry base class, which is everything-but-the-audit functions/members. The new js_generic subclasses Base..., to confirm it doesn't use/want the audit/record functions. Everyone else extends DynamicJSEndpointRegistry and sees no change. (This one is technically unnecessary, but I did it anyway)
  • BaseDynamicJSEndpointRegistry now does 2 more things that js_generic was doing, and it wasn't (beautifully, js_generic built and mostly ran succesfully after deleting all of its code and changing its base class, and then a few e2e tests picked up these behavioural discrepancies):
    • Logs execution time of every JS call, controlled by existing (governance-backed) log_execution_metrics option. I think this will be expected for some deployments directly replacing js_generic with app-space JS, but we may want to add some other controls for turning it on/off.
    • Implements get_allowed_verbs so that HTTP OPTIONS requests (and maybe some other requests?) accurately describe the valid verbs for a given path, even when they're JS, rather than just a generic 404.
  • js_generic is exposed in a public header, as a sample registry which could be extended with your own js::Extensions. Called GovernanceDrivenJSRegistry, because nithtwd.

@eddyashton eddyashton requested a review from a team as a code owner December 18, 2024 14:55
@eddyashton eddyashton enabled auto-merge December 19, 2024 15:01
@eddyashton eddyashton added this pull request to the merge queue Dec 19, 2024
Merged via the queue into microsoft:main with commit 8306717 Dec 19, 2024
13 checks passed
@eddyashton eddyashton deleted the js_generic_is_programmability branch December 19, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor js_generic to use new programmability API
2 participants