Modules (T76883) #118
Merged
Modules (T76883) #118
Conversation
This file contains 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
- Make domains top-level, with /v1 and /sys below. Rationale: Domains will
want to version their APIs independently. Domains are unlikely to go away,
so having them at the top-level does not really introduce any restrictions.
Having a private `/{domain}/sys` in parallel with `/{domian}/v1/` (public
API) is shorter and lets us version the internal interface independently
(using sys2 for example).
- Sketch a global config. Make it domain-oriented, so that we can build up
routing around that. Can use yaml features to reuse / share repetitive
elements, or build up the per-domain config from feature-oriented configs
like InitializeSettings.php in production.
- Make domains top-level, with /v1 and /sys below. Rationale: Domains will
want to version their APIs independently. Domains are unlikely to go away,
so having them at the top-level does not really introduce any restrictions.
Having a private `/{domain}/sys` in parallel with `/{domian}/v1/` (public
API) is shorter and lets us version the internal interface independently
(using sys2 for example).
- Sketch a global config. Make it domain-oriented, so that we can build up
routing around that. Can use yaml features to reuse / share repetitive
elements, or build up the per-domain config from feature-oriented configs
like InitializeSettings.php in production.
This adds a test building a correct-looking tree for the full example spec, including sys and other sub-specs. In the process, I discovered some small bugs in swagger-router. Next steps: see https://phabricator.wikimedia.org/T76883
Conflicts: config.example.yaml doc/Implementation.md
Duplicate module symobls should be checked only once - when the module is first loaded. Otherwise, if a module is used in multiple interfaces, false-positive conflicts are going to arise (since the module's symobls will already be present in the map). This commit fixes this issue.
As expected, flat routing simplifies things quite a bit. I took the opportunity to also remove all __proto__ magic, which should also be good for performance.
When encountered in a spec definition, modules are now being loaded. Two types of modules are supported: * npm: such modules are presumed to exist as restbase's dependencies and are directly require()'d * file: these are local modules, for which the path: argument needs to be provided as well; it is the path from the root directory of restbase
Working: - Revision table creation - Fetching revision info from the MW API - Saving a fetched revision
Misc clean-up and fixes. - Moved key_value bucket creation to the parsoid module (as the owner) - Fix KV bucket URIs for table access Now working: - Request latest HTML for page via Parsoid - Saving of this HTML - Retrieval of saved HTML via 'latest' or specific revision
We should probably handle this by setting up proper configs for test.local now that we have the ability to have per-domain configs.
This is now mostly covered by wentToParsoid assertions on logs. For an example, see the parsoid ondemand tests.
This depends on wikimedia/swagger-router#12 to work.
sysdomain was in use with the layered request router. However, with the
move of storage functionality to /{domain}/sys/table, it is no longer
used. Idem for the storage configuration property.
With this patch:
34 passing (27s)
8 failing
- Don't override Parsoid content types any more (no longer needed / correct)
- Adjust local request checks to new URI layout
- Adjust swagger tests to new API; remove put prereqs & related body
assertions
- Remove x-amples count assertion, as it does not actually seem to differ from
failing tests
- Consistently set the tid on return from parsoid content module
The remaining tests fall into two categories:
- missing features: 'bucket info' for /page/{title}/html, and listings at
/page/{title}/html/
- Broken re-rendering of existing content: I think we should go ahead &
implement the key_rev_value bucket idea, and assign the current tid for
re-renders with content changes (template re-expansions).
- Add title & revision listing support - Adjust some more tests In all there are now four failing tests remaining.
Fixes one more test, which leaves three to do. Also: - Adjust expected UUID in spec tests - More verbose / useful logging for 4xx responses, while reducing the log level for successful responses to 'trace/response'.
This bucket indexes by MediaWiki revision along with the tid. This allows direct access to content by MW revision id, which speeds up the common case. Grouping by revision id also lets us use the current tid for our renders, which more truthfully reflects the current 'as of now' template & other resource usage. For revision deletion & suppression we'll still need to check some other index structure, as it doesn't really make sense to store this separately on each property. One option would be to maintain a list or bloom index of revdeleted revision ids in memory, and then check with the revision table for the actual restrictions. The other option would be to simply perform a query against the revision table in parallel with the main request, although that would likely give away some of the performance gains.
The new key_rev_value code does not create deterministic uuids any more, so adjust the tests to check using dynamic uuids instead. Mocha strongly discourages sharing of state between tests & by default resets shared variables for each test. This means that we need to combine all the re-render tests in a single mocha test in order to share dynamic tids.
This does not seem to be implemented yet, so this test fails. We need to fix this before the release, but can do so after the general merge.
Conflicts: config.example.yaml
|
Changes Unknown when pulling 963f58f on modules into * on master*. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR has been opened to track our progress on T76883. Essentially the same as PR #97 with the difference that we operate on a branch of the main repo.