Delete BaseHTTPService and implement new BaseXmlService (affects [eclipse-marketplace f-droid]; also testing on [uptimerobot circleci]) #2037
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 |
|---|---|---|
|
|
@@ -50,18 +50,6 @@ checkErrorResponse.asPromise = function(errorMessages = {}) { | |
| } | ||
| } | ||
|
|
||
| module.exports = { | ||
| checkErrorResponse, | ||
| } | ||
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 |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| 'use-strict' | ||
|
|
||
| const { InvalidResponse } = require('../services/errors') | ||
| const xml2js = require('xml2js') | ||
|
|
||
| async function asJson({ buffer, res }) { | ||
| try { | ||
| return JSON.parse(buffer) | ||
| } catch (err) { | ||
| throw new InvalidResponse({ | ||
| prettyMessage: 'unparseable json response', | ||
| underlyingError: err, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| async function asXml({ buffer, res }) { | ||
| let xml | ||
| xml2js.parseString(buffer, (err, parsedData) => { | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. Aw, that is a sadly unpleasant thread. Two thoughts:
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 …
}There was a problem hiding this comment. I tied solution 1, switching to
Bad point:
I ran |
||
| if (err != null) { | ||
| throw new InvalidResponse({ | ||
| prettyMessage: 'unparseable xml response', | ||
| underlyingError: err, | ||
| }) | ||
| } | ||
| xml = parsedData | ||
| }) | ||
| return xml | ||
| } | ||
|
|
||
| module.exports = { | ||
| asJson, | ||
| asXml, | ||
| } | ||
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 |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| 'use strict' | ||
|
|
||
| const { expect } = require('chai') | ||
| const { asJson, asXml } = require('./response-parsers') | ||
| const { InvalidResponse } = require('../services/errors') | ||
|
|
||
| describe('Json parser', function() { | ||
| it('parses json', async function() { | ||
| const json = await asJson({ buffer: '{"foo": "bar"}' }) | ||
|
|
||
| expect(json).to.deep.equal({ foo: 'bar' }) | ||
| }) | ||
|
|
||
| it('thows invalid response if json parsing fails', async function() { | ||
| try { | ||
| await asJson({ buffer: 'not json' }) | ||
| expect.fail('Expected to throw') | ||
| } catch (e) { | ||
| expect(e).to.be.an.instanceof(InvalidResponse) | ||
| expect(e.message).to.equal( | ||
| 'Invalid Response: Unexpected token o in JSON at position 1' | ||
| ) | ||
| expect(e.prettyMessage).to.equal('unparseable json response') | ||
| } | ||
| }) | ||
|
|
||
| describe('Xml parser', function() { | ||
| it('parses xml', async function() { | ||
| const xml = await asXml({ buffer: '<foo>bar</foo>' }) | ||
|
|
||
| expect(xml).to.deep.include({ foo: 'bar' }) | ||
| }) | ||
|
|
||
| it('thows invalid response if xml parsing fails', async function() { | ||
| try { | ||
| await asXml({ buffer: 'not xml' }) | ||
| expect.fail('Expected to throw') | ||
| } catch (e) { | ||
| expect(e).to.be.an.instanceof(InvalidResponse) | ||
| expect(e.message).to.equal( | ||
| 'Invalid Response: Non-whitespace before first tag.\nLine: 0\nColumn: 1\nChar: n' | ||
| ) | ||
| expect(e.prettyMessage).to.equal('unparseable xml response') | ||
| } | ||
| }) | ||
| }) | ||
|
There was a problem hiding this comment. 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 |
||
| }) | ||
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
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 |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| 'use strict' | ||
|
|
||
| // See available emoji at http://emoji.muan.co/ | ||
| const emojic = require('emojic') | ||
| const { asXml } = require('../lib/response-parsers') | ||
| const BaseService = require('./base') | ||
| const trace = require('./trace') | ||
|
|
||
| class BaseXmlService extends BaseService { | ||
| async _requestXml({ schema, url, options = {}, errorMessages = {} }) { | ||
| const logTrace = (...args) => trace.logTrace('fetch', ...args) | ||
| const mergedOptions = { | ||
| ...{ headers: { Accept: 'application/xml, text/xml' } }, | ||
| ...options, | ||
| } | ||
| const xmlData = await this._request({ url, mergedOptions, errorMessages }) | ||
| const xml = await asXml(xmlData) | ||
| logTrace(emojic.dart, 'Response XML (before validation)', xml, { | ||
| deep: true, | ||
| }) | ||
| return this.constructor._validate(xml, schema) | ||
| } | ||
| } | ||
|
|
||
| module.exports = BaseXmlService |
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.
We're only using this code in one place. Would it be more direct to move it there, inline?
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.
With
fast-xml-parser, the parser is now used in two places, so keeping therequireup there. 😄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.
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_requestXmethods in the base services?