Delete BaseHTTPService and implement new BaseXmlService (affects [eclipse-marketplace f-droid]; also testing on [uptimerobot circleci]) #2037

PyvesB · 2018-09-01T10:59:19Z

There's quite a few things going on here:

as a follow up to new badge: f-droid #1965 (comment), BaseHTTPService was removed by pushing up the _request function to the BaseService class. Tests were added for this method.
following a similar reasoning, _validate was also pushed up to BaseService, as this allowed to easily implement BaseXmlService alongside the existing BaseJsonService class with very little code duplication.
asJson was pulled out of error-helpers.js and now sits in response-parsers.js with a new friend, asXml. These functions are now properly tested.
the eclipse-markteplace service was broken down and migrated to use the new BaseXmlService class.
calls were migrated to async/await were relevant, following Consistent use of async/await #2028 (comment).

I'll launch a Heroku review app so we can play around with the migrated Eclipse badges and make sure we haven't missed anything! 👍

Eclipse Markteplace badges were migrated as an example

shields-ci · 2018-09-01T11:00:22Z

	Warnings
⚠️	This PR modified helper functions in `lib/` but not accompanying tests. That's okay so long as it's refactoring existing code.
⚠️	This PR modified service code for `f-droid` but not its test code. That's okay so long as it's refactoring existing code.

	Messages
📖	✨ Thanks for your contribution to Shields, @PyvesB!
📖	Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS

PyvesB · 2018-09-01T11:01:51Z

lib/response-parsers.js

+
+async function asXml({ buffer, res }) {
+  let xml
+  xml2js.parseString(buffer, (err, parsedData) => {


This uses a callback and works fine because the underlying implementation is not async (see Leonidas-from-XIV/node-xml2js#159 (comment)), but there is probably a better way of doing this. Any ideas/guidance?

Aw, that is a sadly unpleasant thread. ☹️

Two thoughts:

For the new services, we could adopt the parser mentioned here, which is synchronous: Sync version of parseString Leonidas-from-XIV/node-xml2js#159 (comment) They say it's faster.

You could wrap the callback function in a promise using util.promisify:

const { promisify } = require('util') const parseXml = promisify(require('xml2js').parseString) let xml try { // `parseXml` is returning an awaitable promise so if you were assigning its // return value to a variable, you'd need `await`. However on a return from // an async function, there's no need to specify `await`. return parseXml(buffer) } catch (err) { throw … }

I tied solution 1, switching to fast-xml-parser.
Good points:

cleaner code in response-parser.js.

xml2js has a tendency of introducing arrays everywhere even when not necessary. Switching has simplified the Joi schemas and the constructed JS objects.

claimed to be faster.

Bad point:

~~no helpful exception message if the XML is not parsable.~~

I ran npm i fast-xml-parser to update package-lock.json, is this the correct approach? A huge diff is generated on the file.

PyvesB · 2018-09-01T11:20:51Z

Badges generated for my plugin Notepad4e, using the Heroku review app:

paulmelnikow

This looks great. Nice work! I like how you moved _validate to the base class, too.

I added [uptimerobot circleci] to the PR title – which are just a couple examples of services that use BaseJsonService, which was also affected by this change.

paulmelnikow · 2018-09-01T17:26:47Z

lib/response-parsers.js

+'use strict'
+
+const { InvalidResponse } = require('../services/errors')
+const xml2js = require('xml2js')


We're only using this code in one place. Would it be more direct to move it there, inline?

With fast-xml-parser, the parser is now used in two places, so keeping the require up there. 😄

Ah, sorry, I meant the functions themselves, not the requires. Instead of having these in a separate module, could they just be inlined into the _requestX methods in the base services?

paulmelnikow · 2018-09-01T17:38:08Z

lib/response-parsers.js

+
+async function asXml({ buffer, res }) {
+  let xml
+  xml2js.parseString(buffer, (err, parsedData) => {


Aw, that is a sadly unpleasant thread. ☹️

Two thoughts:

For the new services, we could adopt the parser mentioned here, which is synchronous: Sync version of parseString Leonidas-from-XIV/node-xml2js#159 (comment) They say it's faster.

You could wrap the callback function in a promise using util.promisify:

const { promisify } = require('util') const parseXml = promisify(require('xml2js').parseString) let xml try { // `parseXml` is returning an awaitable promise so if you were assigning its // return value to a variable, you'd need `await`. However on a return from // an async function, there's no need to specify `await`. return parseXml(buffer) } catch (err) { throw … }

paulmelnikow · 2018-09-01T17:44:58Z

lib/response-parsers.spec.js

+        expect(e.prettyMessage).to.equal('unparseable xml response')
+      }
+    })
+  })


Thanks for adding these tests! Well worth keeping and adjusting if this functionality is moved inline, which would widen the bracket a bit.

We aren't using nock directly anywhere right now, though it's probably a good idea to start. I've written service tests for things that ought to be unit tests, and smaller-bracket unit tests for things, e.g. the one in #2036, which probably should mock an HTTP response.

paulmelnikow · 2018-09-01T17:45:34Z

services/base-json.js

+    logTrace(emojic.dart, 'Response JSON (before validation)', json, {
+      deep: true,
+    })
+    return this.constructor._validate(json, schema)


Wow, this is much clearer!

paulmelnikow · 2018-09-01T17:58:47Z

services/base-xml.spec.js

+    expect(serviceData).to.deep.equal({
+      message: 'some-string',
+    })
+  })


Should we add a third test, of valid XML which fails the schema, to establish that _validate is wired up correctly?

paulmelnikow · 2018-09-01T18:01:52Z

services/eclipse-marketplace/eclipse-marketplace-downloads.service.js

+
+    async handle({ name }) {
+      const { marketplace } = await this.fetch({ name, schema })
+      const downloads = base.endsWith('dt')


It might be slightly clearer to condition on interval here.

paulmelnikow · 2018-09-01T18:02:32Z

services/eclipse-marketplace/eclipse-marketplace-downloads.tester.js

@@ -0,0 +1,38 @@
+'use strict'


Were there any changes in the tests, or were they just moved?

The five existing tests we copy-pasted as is in the different tester classes. Additionally, I added one "not found" test case for each implementation.

paulmelnikow · 2018-09-01T18:03:35Z

services/base.js

+      )
+      return value
+    }
+  }


I love the idea of moving _validate here. Nice work!

paulmelnikow · 2018-09-01T18:03:46Z

services/base.js

+    logTrace(emojic.bowAndArrow, 'Request', url, '\n', options)
+    const { res, buffer } = await this._sendAndCacheRequest(url, options)
+    logTrace(emojic.dart, 'Response status code', res.statusCode)
+    return checkErrorResponse.asPromise(errorMessages)({ buffer, res })


AFAICT the only difference between json and xml is the parsing – is that right?

Another way we could consider handling the relationship between _request and _validate is to have _request delegate to a _parse function, and put the format-specific bits there. The service code would invoke _request which would be BaseService's implementation, and that would in turn invoke _parse which would be BaseXxService's.

This would force me to change slightly the way I handled #2031 because every service would then need a _parse implementation. It would also generally force everything to be a native object that can be checked by a schema. However I don't see a problem with that. We could make a BaseSvgBadge subclass easily enough. And encouraging schema validation is good.

I like this idea because the _validate function is in BaseService so it would feel natural to call it from its _request function. The logging code probably could be moved from BaseXxService to BaseService as well.

The requests are slightly different as well, in particular the Accept headers. If we go with your suggestion, we'll also have to add a get defaultHeaders function or something like that, which we would override in the implementations.

Ah, right, the request options, that's a good point. The headers could also be added in a _request override. Though the differences in the requests may mean it's clearer to handle this the way it is.

Conflicts: services/base-json.js services/base-json.spec.js

paulmelnikow · 2018-09-01T19:49:28Z

services/base.js

+        error.message
+      )
+      throw new InvalidResponse({
+        prettyMessage: 'invalid data response',


How about invalid response data?

paulmelnikow · 2018-09-02T19:03:47Z

package-lock.json

@@ -10,7 +10,7 @@
      "integrity": "sha512-OfC2uemaknXr87bdLUkWog7nYuliM9Ij5HUcajsVcMCpQrcLmtxRbVFTIqmcSkSeYRBFBRxs2FiUqFJDLdiebA==",
      "dev": true,
      "requires": {
-        "@babel/highlight": "^7.0.0"
+        "@babel/highlight": "7.0.0"


This package-lock diff is unusually large. Could you make sure you're on the latest npm, reset this file, and then run npm install again to see if that tightens it up?

Yep, updating to the latest version of npm seems to have helped a lot. 😉

paulmelnikow

Clarified one (prios) suggestion, though happy to defer that, too! This looks great!

PyvesB · 2018-09-02T20:18:54Z

@paulmelnikow I've tried inlining the code as per your suggestion, but I think I'm getting confused:

async _requestJson({ schema, url, options = {}, errorMessages = {} }) {
    const logTrace = (...args) => trace.logTrace('fetch', ...args)
    const mergedOptions = {
      ...{ headers: { Accept: 'application/json' } },
      ...options,
    }
    const jsonData = await this._request({
      url,
      options: mergedOptions,
      errorMessages,
    })
    let json
    try {
      json = JSON.parse(jsonData)
    } catch (err) {
      throw new InvalidResponse({
        prettyMessage: 'unparseable json response',
        underlyingError: err,
      })
    }
    logTrace(emojic.dart, 'Response JSON (before validation)', json, {
      deep: true,
    })
    return this.constructor._validate(json, schema)
  }

All JSON tests have started failing with unparseable json response. Am I somehow being thick?

paulmelnikow · 2018-09-03T06:21:39Z

Does checkErrorResponse return just th buffer? If so the code looks fine. You could try logging jsonData. Do the service tests work? Could the problem be with the unit tests themselves?

paulmelnikow · 2018-09-03T06:23:53Z

Ah, no, I think it returns { buffer, res }. I think you want const { buffer } = await this._request(...).

PyvesB · 2018-09-03T18:03:43Z

Indeed, I was getting confused with the return types, thanks for the help. This should now be ready to go! 👍

shields-deployment · 2018-09-03T18:37:41Z

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

Deleted BaseHTTPService and implemented new BaseXmlService

06dac0e

Eclipse Markteplace badges were migrated as an example

PyvesB added service-badge New or updated service badge developer-experience Dev tooling, test framework, and CI core Server, BaseService, GitHub auth, Shared helpers labels Sep 1, 2018

PyvesB commented Sep 1, 2018

View reviewed changes

Fixed lint errors

b9b6a84

Switched to dynamic date for badge example

5ec6bfe

paulmelnikow temporarily deployed to shields-staging-pr-2037 September 1, 2018 11:21 Inactive

PyvesB changed the title ~~Delete BaseHTTPService and implement new BaseXmlService~~ Delete BaseHTTPService and implement new BaseXmlService (affects [eclipse-marketplace f-droid]) Sep 1, 2018

paulmelnikow reviewed Sep 1, 2018

View reviewed changes

paulmelnikow changed the title ~~Delete BaseHTTPService and implement new BaseXmlService (affects [eclipse-marketplace f-droid])~~ Delete BaseHTTPService and implement new BaseXmlService (affects [eclipse-marketplace f-droid]; also testing on [uptimerobot circleci]) Sep 1, 2018

Merge remote-tracking branch 'upstream/master' into refactor-base-xml

ea91b5f

Conflicts: services/base-json.js services/base-json.spec.js

paulmelnikow temporarily deployed to shields-staging-pr-2037 September 1, 2018 19:38 Inactive

Fixed tests

5a0737a

paulmelnikow temporarily deployed to shields-staging-pr-2037 September 1, 2018 19:45 Inactive

paulmelnikow reviewed Sep 1, 2018

View reviewed changes

Switched to fast-xml-parser

78cede5

paulmelnikow temporarily deployed to shields-staging-pr-2037 September 1, 2018 20:54 Inactive

Based check on interval

7bc9c0b

paulmelnikow temporarily deployed to shields-staging-pr-2037 September 1, 2018 21:03 Inactive

Strengthened tests

5de279e

paulmelnikow temporarily deployed to shields-staging-pr-2037 September 1, 2018 21:20 Inactive

Made phrasing clearer

4dd47ec

paulmelnikow temporarily deployed to shields-staging-pr-2037 September 1, 2018 21:21 Inactive

Removed extension

3dbbce2

paulmelnikow temporarily deployed to shields-staging-pr-2037 September 1, 2018 21:26 Inactive

Merge branch 'master' into refactor-base-xml

9bf7c33

paulmelnikow temporarily deployed to shields-staging-pr-2037 September 1, 2018 21:28 Inactive

paulmelnikow mentioned this pull request Sep 2, 2018

[WIP] Rewrite [vso] badges #2031

Closed

paulmelnikow reviewed Sep 2, 2018

View reviewed changes

paulmelnikow previously approved these changes Sep 2, 2018

View reviewed changes

Updated package-lock.json with latest version of npm

5dc426d

PyvesB dismissed paulmelnikow’s stale review via 5dc426d September 2, 2018 19:50

paulmelnikow temporarily deployed to shields-staging-pr-2037 September 2, 2018 19:50 Inactive

paulmelnikow previously approved these changes Sep 2, 2018

View reviewed changes

Merge branch 'master' into refactor-base-xml

45f9e8b

paulmelnikow temporarily deployed to shields-staging-pr-2037 September 2, 2018 19:55 Inactive

PyvesB mentioned this pull request Sep 2, 2018

Update tutorial to reflect new service layout #2042

Merged

Inlined JSON and XML parsers

a0de9f9

PyvesB dismissed paulmelnikow’s stale review via a0de9f9 September 3, 2018 17:59

paulmelnikow temporarily deployed to shields-staging-pr-2037 September 3, 2018 17:59 Inactive

Merge branch 'master' into refactor-base-xml

372a382

paulmelnikow deployed to shields-staging-pr-2037 September 3, 2018 17:59 View deployment

paulmelnikow approved these changes Sep 3, 2018

View reviewed changes

PyvesB merged commit 7417dc5 into badges:master Sep 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete BaseHTTPService and implement new BaseXmlService (affects [eclipse-marketplace f-droid]; also testing on [uptimerobot circleci]) #2037

Delete BaseHTTPService and implement new BaseXmlService (affects [eclipse-marketplace f-droid]; also testing on [uptimerobot circleci]) #2037

PyvesB commented Sep 1, 2018

shields-ci commented Sep 1, 2018 •

edited

Loading

PyvesB Sep 1, 2018

paulmelnikow Sep 1, 2018

PyvesB Sep 1, 2018 •

edited

Loading

PyvesB commented Sep 1, 2018 •

edited

Loading

paulmelnikow left a comment

paulmelnikow Sep 1, 2018

PyvesB Sep 1, 2018

paulmelnikow Sep 2, 2018

paulmelnikow Sep 1, 2018

paulmelnikow Sep 1, 2018

paulmelnikow Sep 1, 2018

paulmelnikow Sep 1, 2018

paulmelnikow Sep 1, 2018

paulmelnikow Sep 1, 2018

PyvesB Sep 1, 2018

paulmelnikow Sep 1, 2018

paulmelnikow Sep 1, 2018

PyvesB Sep 1, 2018 •

edited

Loading

paulmelnikow Sep 1, 2018

paulmelnikow Sep 1, 2018

paulmelnikow Sep 2, 2018

PyvesB Sep 2, 2018

paulmelnikow left a comment

PyvesB commented Sep 2, 2018

paulmelnikow commented Sep 3, 2018

paulmelnikow commented Sep 3, 2018

PyvesB commented Sep 3, 2018

shields-deployment bot commented Sep 3, 2018

Delete BaseHTTPService and implement new BaseXmlService (affects [eclipse-marketplace f-droid]; also testing on [uptimerobot circleci]) #2037

Delete BaseHTTPService and implement new BaseXmlService (affects [eclipse-marketplace f-droid]; also testing on [uptimerobot circleci]) #2037

Conversation

PyvesB commented Sep 1, 2018

shields-ci commented Sep 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PyvesB Sep 1, 2018 • edited Loading

Choose a reason for hiding this comment

PyvesB commented Sep 1, 2018 • edited Loading

paulmelnikow 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

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

PyvesB Sep 1, 2018 • 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

paulmelnikow left a comment

Choose a reason for hiding this comment

PyvesB commented Sep 2, 2018

paulmelnikow commented Sep 3, 2018

paulmelnikow commented Sep 3, 2018

PyvesB commented Sep 3, 2018

shields-deployment bot commented Sep 3, 2018

shields-ci commented Sep 1, 2018 •

edited

Loading

PyvesB Sep 1, 2018 •

edited

Loading

PyvesB commented Sep 1, 2018 •

edited

Loading

PyvesB Sep 1, 2018 •

edited

Loading