Remove MWCapabilities object (partial impl) #1878
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,26 +11,126 @@ import basicURLDirector from './util/builders/url/basic.director.js' | |
| import BaseURLDirector from './util/builders/url/base.director.js' | ||
| import ApiURLDirector from './util/builders/url/api.director.js' | ||
|
|
||
| interface MWCapabilities { | ||
| apiAvailable: boolean | ||
| veApiAvailable: boolean | ||
| coordinatesAvailable: boolean | ||
| desktopRestApiAvailable: boolean | ||
| } | ||
| class MediaWiki { | ||
| public metaData: MWMetaData | ||
| public readonly baseUrl: URL | ||
| public readonly modulePathConfig | ||
| public readonly getCategories: boolean | ||
| public readonly namespaces: MWNamespaces = {} | ||
| public readonly namespacesToMirror: string[] = [] | ||
|
|
||
| private readonly wikiPath: string | ||
| private readonly restApiPath: string | ||
| private readonly username: string | ||
| private readonly password: string | ||
| private readonly apiPath: string | ||
| private readonly domain: string | ||
| private apiUrlDirector: ApiURLDirector | ||
| private baseUrlDirector: BaseURLDirector | ||
|
|
||
| private _veapiUrl: URL | ||
| private _restApiUrl: URL | ||
| private _apiUrl: URL | ||
| private _modulePath: string | ||
| private _webUrl: URL | ||
| private _desktopRestApiUrl: URL | ||
| // TODO: Mobile builder was removed since there is no /mobile-sections endpoint | ||
|
|
||
| // Set default MW capabilities | ||
| private readonly mwCapabilities: MWCapabilities | ||
|
|
||
| /** | ||
| * veApiUrl based on top of 'new ApiURLDirecto' | ||
| */ | ||
| public get veapiUrl(): URL { | ||
| if (!this._veapiUrl) { | ||
| // TODO: This depend on baseUrlDirector.buildURL(this.apiPath) and looks like a weak solution | ||
| this._veapiUrl = this.apiUrlDirector.buildVisualEditorURL() | ||
| } | ||
| return this._veapiUrl | ||
|
There was a problem hiding this comment. Not easy, considering that you have so many different styles and developers behind the codebase... but think about:
There was a problem hiding this comment. I'd like that we have a dedicated ticket about naming conventions. There was a problem hiding this comment. For VE, the answer is in the validation of https://github.com/openzim/mwoffliner/wiki/API-end%E2%80%90points. For There was a problem hiding this comment. I renamed |
||
| } | ||
|
|
||
| /** | ||
| * restApiUrl, apiUrl, modulePath, webUrl and desktopRestApiUr are based on top of 'new BaseURLDirector' | ||
| */ | ||
| public get restApiUrl(): URL { | ||
| if (!this._restApiUrl) { | ||
| this._restApiUrl = this.baseUrlDirector.buildRestApiURL(this.restApiPath) | ||
| } | ||
| // TODO: define usage of this property | ||
| return this._restApiUrl | ||
| } | ||
|
|
||
| public get apiUrl(): URL { | ||
| if (!this._apiUrl) { | ||
| this._apiUrl = this.baseUrlDirector.buildURL(this.apiPath) | ||
| } | ||
| return this._apiUrl | ||
| } | ||
|
|
||
| public get modulePath() { | ||
| if (!this._modulePath) { | ||
| this._modulePath = this.baseUrlDirector.buildModuleURL(this.modulePathConfig) | ||
| } | ||
| return this._modulePath | ||
| } | ||
|
|
||
| public get webUrl(): URL { | ||
| if (!this._webUrl) { | ||
| this._webUrl = this.baseUrlDirector.buildURL(this.wikiPath) | ||
| } | ||
| return this._webUrl | ||
| } | ||
|
|
||
| public get desktopRestApiUrl(): URL { | ||
| if (!this._desktopRestApiUrl) { | ||
| this._desktopRestApiUrl = this.baseUrlDirector.buildDesktopRestApiURL(this.restApiPath) | ||
| } | ||
| return this._desktopRestApiUrl | ||
| } | ||
|
|
||
| public hasDesktopRestApi = async function (loginCookie?: string, testArticleId?: string): Promise<any> { | ||
| const desktopRestApiAvailable = await this.checkApiAvailabilty(this.getDesktopRestApiArticleUrl(testArticleId), loginCookie) | ||
| this.hasDesktopRestApi = async function (): Promise<boolean> { | ||
| return desktopRestApiAvailable | ||
| } | ||
| } | ||
|
|
||
| public hasVeApi = async function (loginCookie?: string, testArticleId?: string): Promise<any> { | ||
| const veRestApiAvailable = await this.checkApiAvailabilty(this.getVeApiArticleUrl(testArticleId), loginCookie) | ||
| this.hasVeApi = async function (): Promise<boolean> { | ||
| return veRestApiAvailable | ||
| } | ||
| } | ||
|
|
||
| public hasCoordinatesApi = async function (downloader?: Downloader): Promise<any> { | ||
| const validNamespaceIds = this.namespacesToMirror.map((ns) => this.namespaces[ns].num) | ||
| const reqOpts = { | ||
| action: 'query', | ||
| format: 'json', | ||
| // TODO: Do we need this.mwCapabilities.coordinatesAvailable here? | ||
| prop: `redirects|revisions${this.mwCapabilities.coordinatesAvailable ? '|coordinates' : ''}${this.getCategories ? '|categories' : ''}`, | ||
|
There was a problem hiding this comment. Here there is a chicken-egg problem, not sure how this happened, but you clearly need to put |
||
| rdlimit: 'max', | ||
| rdnamespace: validNamespaceIds.join('|'), | ||
| } | ||
|
|
||
| // TODO: replace|rename|refactor getJSON later | ||
| if (downloader) { | ||
| const resp = await downloader.getJSON<MwApiResponse>(this.apiUrlDirector.buildQueryURL(reqOpts)) | ||
| const isCoordinateWarning = resp.warnings && resp.warnings.query && (resp.warnings.query['*'] || '').includes('coordinates') | ||
| if (isCoordinateWarning) { | ||
| logger.info('Coordinates not available on this wiki') | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| constructor(config: MWConfig) { | ||
| this.domain = config.domain || '' | ||
|
|
@@ -39,23 +139,23 @@ class MediaWiki { | |
| this.getCategories = config.getCategories | ||
|
|
||
| this.baseUrl = basicURLDirector.buildMediawikiBaseURL(config.base) | ||
| this.apiPath = config.apiPath ?? 'w/api.php' | ||
| this.wikiPath = config.wikiPath ?? DEFAULT_WIKI_PATH | ||
| this.restApiPath = config.restApiPath | ||
| this.modulePathConfig = config.modulePath | ||
|
|
||
| // Instantiate Url Directors | ||
| this.baseUrlDirector = new BaseURLDirector(this.baseUrl.href) | ||
| this.apiUrlDirector = new ApiURLDirector(this.apiUrl.href) | ||
|
|
||
| // Default capabilities | ||
| // TODO: check whether to remove this object | ||
|
There was a problem hiding this comment. It seems you are almost done and ready to get rid of this |
||
| this.mwCapabilities = { | ||
| apiAvailable: false, | ||
| veApiAvailable: false, | ||
| coordinatesAvailable: true, | ||
| desktopRestApiAvailable: false, | ||
| } | ||
| } | ||
|
|
||
| public async login(downloader: Downloader) { | ||
|
|
@@ -85,12 +185,17 @@ class MediaWiki { | |
| }, | ||
| method: 'POST', | ||
| }) | ||
| .then(async (resp) => { | ||
| if (resp.data.login.result !== 'Success') { | ||
| throw new Error('Login Failed') | ||
| } | ||
|
|
||
| /* | ||
| TODO: Cookie is shared between Downloader and Mediawiki, probably antipattern. Use as interim solution for now. | ||
| Also, double-check possible race condition - cookies should be set before checking capabilities. | ||
| */ | ||
| downloader.loginCookie = resp.headers['set-cookie'].join(';') | ||
|
There was a problem hiding this comment. Indee... here would help to clearly define what is the duty of the |
||
| await this.checkCapabilities(resp.headers['set-cookie'].join(';')) | ||
| }) | ||
| .catch((err) => { | ||
| throw err | ||
|
|
@@ -296,6 +401,12 @@ class MediaWiki { | |
|
|
||
| return mwMetaData | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. A few comments have been lost here, maybe on purpose?! Just try to keep things commented when needed. |
||
| private async checkCapabilities(loginCookie?: string, testArticleId = 'MediaWiki:Sidebar'): Promise<void> { | ||
| await this.hasDesktopRestApi(loginCookie, testArticleId) | ||
| await this.hasVeApi(loginCookie, testArticleId) | ||
| await this.hasCoordinatesApi() | ||
| } | ||
| } | ||
|
|
||
| export default MediaWiki | ||
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
veApiUrl(),restApiUrl(), etc. are shortcuts and offer lazyness to the URL directors. I see little value to have them. I suggest to just something like this.apiUrlDirector.getVisualEditorURL()` which works in a lazy manner.If you you think
this.apiUrlDirector.getVisualEditorURL()is really too cumbersome, then OK to just keep the redirection part, but I see no reason why the lazyness is not built in the Director itself considering that the configuration of it happens at construction time.