Update setBaseUrl() to handle URL directors #1929

VadimKovalenkoSNF · 2023-10-17T08:41:03Z

Fixes: #1927
Depends on #1939
Depends on #1944

codecov · 2023-10-17T09:22:52Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (cb4f7a2) 74.17% compared to head (db23e5c) 73.89%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1929      +/-   ##
==========================================
- Coverage   74.17%   73.89%   -0.29%     
==========================================
  Files          39       39              
  Lines        3106     3095      -11     
  Branches      686      679       -7     
==========================================
- Hits         2304     2287      -17     
- Misses        687      689       +2     
- Partials      115      119       +4

Files	Coverage Δ
src/Downloader.ts	`71.42% <100.00%> (-0.73%)`	⬇️
src/config.ts	`100.00% <ø> (ø)`
src/renderers/abstractMobile.render.ts	`95.45% <100.00%> (ø)`
src/util/builders/url/api.director.ts	`87.50% <100.00%> (-1.98%)`	⬇️
src/util/builders/url/visual-editor.director.ts	`100.00% <100.00%> (ø)`
src/mwoffliner.lib.ts	`75.45% <0.00%> (+0.64%)`	⬆️
src/util/saveArticles.ts	`75.72% <66.66%> (-0.12%)`	⬇️
src/MediaWiki.ts	`87.11% <94.59%> (+0.35%)`	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

VadimKovalenkoSNF · 2023-10-17T14:00:33Z

The idea behind this patch is to remove downloader.setBaseUrl as this functionality is redundant and duplicates URL builder methods which can be used directly in saveArticles() loop to create the correct article Url using <render constructor name>.buildArticleURL(articleId)

kelson42

Not the proper architecture:

CPU inefficient, why testing millions of times something which can be decided once for all?
The overall concept of having classes sharing the same API and they are interchangeable is always underused, although this is a base principle of the new architecture
Director in a technicality which should be in the Downloader... and ideally nowhere else.
To me it looks like a serious regression has been introduced by not handling the case with anymore when forceRender is null.

kelson42 · 2023-10-18T05:41:12Z

src/Downloader.ts

+  public getArticleUrl(renderer, articleId: string): string {
+    let articleUrl: string
+    // Renders and URL builders are independent so there should be a switch/case scenario here
+    switch (renderer.constructor.name) {
+      case 'WikimediaDesktopRenderer':
+        articleUrl = MediaWiki.wikimediaDesktopUrlDirector.buildArticleURL(articleId)
+        break
+      case 'VisualEditorRenderer':
+        articleUrl = MediaWiki.visualEditorURLDirector.buildArticleURL(articleId)
+        break
+      case 'WikimediaMobileRenderer':
+        articleUrl = MediaWiki.wikimediaMobileUrlDirector.buildArticleURL(articleId)
+        break


We are not going do go through this millions of times, if this is can be done once per scrape. Decide which Director to use once for all.

Ok, this has been revamped in favor of using URL builder's .buildArticleURL(articleId) method.

kelson42 · 2023-10-18T05:44:04Z

src/Downloader.ts

-  public async setBaseUrls(forceRender = null) {
-    if (!forceRender) {
-      //* Objects order in array matters!
-      this.baseUrl = basicURLDirector.buildDownloaderBaseUrl([
-        { condition: await MediaWiki.hasWikimediaMobileApi(), value: MediaWiki.WikimediaMobileApiUrl.href },
-        { condition: await MediaWiki.hasWikimediaDesktopApi(), value: MediaWiki.WikimediaDesktopApiUrl.href },
-        { condition: await MediaWiki.hasVisualEditorApi(), value: MediaWiki.visualEditorApiUrl.href },
-      ])
-
-      //* Objects order in array matters!
-      this.baseUrlForMainPage = basicURLDirector.buildDownloaderBaseUrl([
-        { condition: await MediaWiki.hasWikimediaDesktopApi(), value: MediaWiki.WikimediaDesktopApiUrl.href },
-        { condition: await MediaWiki.hasVisualEditorApi(), value: MediaWiki.visualEditorApiUrl.href },
-        { condition: await MediaWiki.hasWikimediaMobileApi(), value: MediaWiki.WikimediaMobileApiUrl.href },
-      ])
-    } else {


Where goes all this logic to decide automatically which API/render to use? Init the Director object at construction time. You can not get ride of a some kind of init code for this.

The logic for choosing render is in saveArticles(). Here you can find a condition where forceRender is null. When choosing render, src/renderers/renderer.builder.ts automatically selects the necessary API based on the predefined renderType option.

Upd: I brought back this logic + calculate the necessary URL builder here.

kelson42 · 2023-10-18T05:51:48Z

src/Downloader.ts

-        default:
-          throw new Error('Unable to find specific API end-point to retrieve article HTML')
-      }
+  public getArticleUrl(renderer, articleId: string): string {


This method should have only one argument: articleId

Instead of renderer I passed dump to apply isMainPage condition, pls check.

src/MediaWiki.ts

kelson42 · 2023-10-18T05:56:11Z

src/MediaWiki.ts

@@ -152,7 +152,7 @@ class MediaWiki {

  public async hasVisualEditorApi(): Promise<boolean> {
    if (this.#hasVisualEditorApi === null) {
-      this.#hasVisualEditorApi = await checkApiAvailability(this.VisualEditorURLDirector.buildArticleURL(this.apiCheckArticleId))
+      this.#hasVisualEditorApi = await checkApiAvailability(this.visualEditorURLDirector.buildArticleURL(this.apiCheckArticleId))


Instanciate the needed director here temporary if you need it.

kelson42 · 2023-10-18T05:56:31Z

src/MediaWiki.ts

    this.wikimediaDesktopUrlDirector = new WikimediaDesktopURLDirector(this.WikimediaDesktopApiUrl.href)
    this.wikimediaMobileUrlDirector = new WikimediaMobileURLDirector(this.WikimediaMobileApiUrl.href)
-    this.VisualEditorURLDirector = new VisualEditorURLDirector(this.visualEditorApiUrl.href)
+    this.visualEditorURLDirector = new VisualEditorURLDirector(this.visualEditorApiUrl.href)


Should vanish

Done, now instantiates in related Mediawiki hasCapabilies lazy methods.

src/util/builders/url/api.director.ts

kelson42 · 2023-10-26T04:44:17Z

@VadimKovalenkoSNF Unfortunately didn't have the time to look at this in detail. But please rebase and fix conflict and I will within 24 hours.

kelson42

We are not there yet unfortunately. See my comments.

kelson42 · 2023-10-29T12:38:26Z

src/Downloader.ts

+        { condition: await MediaWiki.hasVisualEditorApi(), value: MediaWiki.VisualEditorApiUrl.href, Director: VisualEditorURLDirector },
+        { condition: await MediaWiki.hasWikimediaMobileApi(), value: MediaWiki.WikimediaMobileApiUrl.href, Director: WikimediaMobileURLDirector },
+      ]
+      this.baseUrlForMainPage = basicURLDirector.buildDownloaderBaseUrl(mainPageCapabilitiesList)


Why we stick with a baseUrl here and don't have a director like for other articles? Should be the case IMO. We should not have anything about baseUrl here in this class and probably nowhere else probably.

Pls check recent changes, the property Downloader.baseUrl is no longer used.

kelson42 · 2023-10-29T13:03:51Z

src/Downloader.ts

+      const articlesCapabilitiesList = [
+        { condition: await MediaWiki.hasWikimediaMobileApi(), value: MediaWiki.WikimediaMobileApiUrl.href, Director: WikimediaMobileURLDirector },
+        { condition: await MediaWiki.hasWikimediaDesktopApi(), value: MediaWiki.WikimediaDesktopApiUrl.href, Director: WikimediaDesktopURLDirector },
+        { condition: await MediaWiki.hasVisualEditorApi(), value: MediaWiki.VisualEditorApiUrl.href, Director: VisualEditorURLDirector },
+      ]
+
+      this.baseUrl = basicURLDirector.buildDownloaderBaseUrl(articlesCapabilitiesList)
+      this.articleUrlDirector = this.getUrlDirector(articlesCapabilitiesList)


Why the condition does not returns directly the director.... like it was returning directly the baseURL in the past? Concept of capabilites (and related functions) seem useless.

I refactored this to downloader.setUrlsDirectors(mainPageRenderer, articlesRenderer)

kelson42 · 2023-10-29T13:22:02Z

src/MediaWiki.ts

@@ -181,15 +184,12 @@ class MediaWiki {
    const baseUrlDirector = new BaseURLDirector(this.baseUrl.href)


This whole function/method initMWApis() should disappear:

baseUrlDirector should be make class public variable a this.urlDirector

BaseURLDirector should take all the necessary arguments to be able to deliver all the necessary paths:buildWikimediaDesktopApiUrl, buildVisualEditorURL ...

BaseURLDirector method buildWikimediaDesktopApiUrl, buildVisualEditorURL ... should be made lazy and renamed in getWikimediaDekstopApiUrl(), ....

Mediawiki method urlDirector should be used in place of all these variables.

I refactored this part of the code, pls check set base(value: string) { ... }. Once we set this.baseUrl in Mediawiki we are eligible to calculate other endpoints.

kelson42 · 2023-10-29T13:30:45Z

src/Downloader.ts


-    if (!this.baseUrl || !this.baseUrlForMainPage) throw new Error('Unable to find appropriate API end-point to retrieve article HTML')
+  public getArticleUrl(dump: Dump, articleId: string): string {


Don't take dump as parameter, only the articleId. Create in addition a getMainPageUrl(articleId)... the function interface naming is not great I know....

Created downloader.getMainPageUrl()

kelson42 · 2023-11-13T06:26:41Z

@VadimKovalenkoSNF This PR should be rebased and hopefuly is ready to review

…ArticleUrl()

…ctors()

VadimKovalenkoSNF force-pushed the 1927-fix-article-url-calculation branch from a52ea6c to 3a5526e Compare October 17, 2023 09:10

VadimKovalenkoSNF force-pushed the 1927-fix-article-url-calculation branch from c0b1112 to c5fe712 Compare October 17, 2023 13:46

VadimKovalenkoSNF marked this pull request as ready for review October 17, 2023 14:13

VadimKovalenkoSNF requested a review from kelson42 October 17, 2023 14:13

kelson42 requested changes Oct 18, 2023

View reviewed changes

VadimKovalenkoSNF changed the title ~~Deprecate setBaseUrl() and getArticleUrl() in favor of downloader.getArticleUrl()~~ Update setBaseUrl() to handle URL directors Oct 23, 2023

VadimKovalenkoSNF requested a review from kelson42 October 23, 2023 14:37

VadimKovalenkoSNF mentioned this pull request Oct 25, 2023

Implement MediaWiki REST API render #1926

Closed

VadimKovalenkoSNF force-pushed the 1927-fix-article-url-calculation branch 2 times, most recently from df90842 to 4e244a4 Compare October 26, 2023 12:24

kelson42 requested changes Oct 29, 2023

View reviewed changes

VadimKovalenkoSNF force-pushed the 1927-fix-article-url-calculation branch 2 times, most recently from 9f313bd to aa14e7a Compare November 9, 2023 12:47

VadimKovalenkoSNF added 10 commits November 13, 2023 22:23

Deprecate setBaseUrl() and getArticleUrl() in favor of downloader.get…

ba9c132

…ArticleUrl()

Add unit test for donwloader.getArticleUrl(), fix VE url builder

9342533

Fix codecov issues in Downloader

b628472

Handle url director instances inside Downloader

916e9d3

Minor fixes after rebase

0f718bc

Fix codefactor issue in mwoffliner.lib.ts

2f5d03f

Fix handling articleId for VE

0d937d1

Add 1000 ms delay to tests where muliple renderers are involved

5b169c1

Deprecate downloader.setBaseUrls() in favor of downloader.setUrlsDire…

d8bf7fc

…ctors()

Refactor MW url builder methods invocation, refactor forceRender test

db23e5c

VadimKovalenkoSNF force-pushed the 1927-fix-article-url-calculation branch from aa14e7a to db23e5c Compare November 13, 2023 20:24

VadimKovalenkoSNF requested a review from kelson42 November 13, 2023 21:17

VadimKovalenkoSNF mentioned this pull request Nov 15, 2023

Implement Mediawiki REST API render #1947

Merged

kelson42 merged commit 31709d3 into main Nov 15, 2023
6 checks passed

kelson42 deleted the 1927-fix-article-url-calculation branch November 15, 2023 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update setBaseUrl() to handle URL directors #1929

Update setBaseUrl() to handle URL directors #1929

VadimKovalenkoSNF commented Oct 17, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

VadimKovalenkoSNF commented Oct 17, 2023

kelson42 left a comment •

edited

Loading

kelson42 Oct 18, 2023

VadimKovalenkoSNF Oct 23, 2023

kelson42 Oct 18, 2023

VadimKovalenkoSNF Oct 18, 2023

VadimKovalenkoSNF Oct 23, 2023

kelson42 Oct 18, 2023

VadimKovalenkoSNF Oct 23, 2023

kelson42 Oct 18, 2023

VadimKovalenkoSNF Oct 23, 2023

kelson42 Oct 18, 2023

VadimKovalenkoSNF Oct 23, 2023

kelson42 commented Oct 26, 2023

kelson42 left a comment

kelson42 Oct 29, 2023

VadimKovalenkoSNF Nov 9, 2023

kelson42 Oct 29, 2023

VadimKovalenkoSNF Nov 9, 2023

kelson42 Oct 29, 2023

VadimKovalenkoSNF Nov 9, 2023

kelson42 Oct 29, 2023 •

edited

Loading

VadimKovalenkoSNF Nov 7, 2023

kelson42 commented Nov 13, 2023

		@@ -181,15 +184,12 @@ class MediaWiki {
		const baseUrlDirector = new BaseURLDirector(this.baseUrl.href)


		if (!this.baseUrl \|\| !this.baseUrlForMainPage) throw new Error('Unable to find appropriate API end-point to retrieve article HTML')
		public getArticleUrl(dump: Dump, articleId: string): string {

Update setBaseUrl() to handle URL directors #1929

Update setBaseUrl() to handle URL directors #1929

Conversation

VadimKovalenkoSNF commented Oct 17, 2023 • edited Loading

codecov bot commented Oct 17, 2023 • edited Loading

Codecov Report

VadimKovalenkoSNF commented Oct 17, 2023

kelson42 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelson42 commented Oct 26, 2023

kelson42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelson42 Oct 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelson42 commented Nov 13, 2023

VadimKovalenkoSNF commented Oct 17, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

kelson42 left a comment •

edited

Loading

kelson42 Oct 29, 2023 •

edited

Loading