Add API for getting diffs #60

Mr0grog · 2017-05-31T07:57:47Z

Fixes #8.

This is currently implemented on top of @b5’s simple diff service: https://github.com/edgi-govdata-archiving/go-calc-diff
…but should theoretically work with any HTTP differ that accepts requests with two query arguments:

a: A URL to the content of the starting version
b: A URL to the content of the ending version

e.g: http://diff-it.com?a=http%3A%2F%2Fexample.com%2Fv1&b=http%3A%2F%2Fexample.com%2Fv2

Diffs are retrieved by requesting the URL:

/api/v0/pages/{page_id}/changes/{version_a_id}..{version_b_id}/diff/{diff_type}?{query_params}

(Meant to mirror the annotations API)

query_params are simply passed along to the underlying diff service. For example, go-calc-diff can take a format argument that determines how the response is formatted (JSON, HTML document, HTML snippet):

/api/v0/pages/xxx/changes/yyy..zzz/diff/source?format=json

That returns a response like:

{
  "data": {
    "page_id": "3f28a7c3-65f2-481e-9722-efccc1f38462",
    "from_version_id": "51896700-93d7-42df-8a3c-aa0282e1c8e8",
    "to_version_id": "16d82acd-01ef-4e39-a5d2-ab11daa6540e",
    "diff_service": "DIFF SERVICE NAME",
    "diff_service_version": "?",
    "content": "RESPONSE FROM DIFF SERVICE HERE"
  }
}

If the response was JSON data, content will actually an object. Otherwise, it will be a string.

We only support one diff_type right now (source), but the system is designed to be expandable. Under the hood, there is a registry that maps diff type names to actual diff service implementations. Implementations only need to respond to one method:

def diff (change_model, options = {})
  # return a string or hash that will be used in the `content` field
end

For now, we just have one SimpleDiff class, which is instantiated with a URL. It can be used to interact with any diff service that matches the same protocol as go-calc-diff.

Mr0grog · 2017-05-31T08:05:47Z

Hoping to get this worked out before Thursday so Global Sprinters have a working example of diffing.

Mr0grog · 2017-05-31T08:10:47Z

Running on staging for testing, e.g:

https://web-monitoring-db-staging.herokuapp.com/api/v0/pages/76d2e99c-c1a5-44e5-9781-6622573b9444/changes/..5ef676d4-0811-4111-aa80-19d2999ab178/diff/source

or https://web-monitoring-db-staging.herokuapp.com/api/v0/pages/76d2e99c-c1a5-44e5-9781-6622573b9444/changes/..5ef676d4-0811-4111-aa80-19d2999ab178/diff/source?format=json (for fancy JSON data)

Mr0grog · 2017-05-31T08:14:55Z

Side note: this includes a wonky patch for an encoding issue in HTTParty (jnunemaker/httparty#542), which was also causing import issues (if raw version content had to be parsed, which is not the case for most imports).

danielballan · 2017-05-31T16:00:52Z

config/initializers/differ.rb

+require_dependency 'differ/simple_diff'
+
+if ENV['DIFFER_SOURCE']
+  Differ.register(:source, Differ::SimpleDiff.new(ENV['DIFFER_SOURCE']))


Would it be too clever to allow arbitrary many differs with arbitrary names to be configured through ENV by picking up all vars with names that match DIFFER_*? Or will new differs likely need something more complex that SimpleDiff? Offhand I'm not sure what they would need so the approach I'm floating seems doable -- but maybe less than clean.

allow arbitrary many differs with arbitrary names to be configured through ENV by picking up all vars with names that match DIFFER_*?

Ha! I thought about the same idea and then said “nah, let’s not get ahead of ourselves.” I think I would at least change the var to DIFFER_SIMPLE_* if we did that.

I can imagine some differs being more complex (or maybe SimpleDiff will just grow)—we can‘t meaningfully diff PDFs with this, for example, but we really should eventually have that capability. There’s a lot of PDF content that is “tracked” but has no real tooling to support analyzing it. Versionista is also hitting some audio files and potentially things like CSVs. I haven't really done a full audit of content types. By nature, I’m sure PF is grabbing who-knows-what variety of content types.

danielballan

I left one stray single comment. I read this through twice and the tests look good. I agree with the general API. Let's merge and iterate.

Similar route to annotations, since both are about a change between two versions: GET /api/v0/page/{page id}/changes/{from id}..{to id}/diff/{diff type}

There is a registry of diffing services that can be set with: Differ.register(type, service) And retrieved with: Differ.for_type(type) # returns nil for unknown type Differ.for_type!(type) # raises for unknown type A diffing service need only respond to the `diff(change_model, options_hash)` method. Right now there is only one diffing service: SimpleDiff. It's configurable with a URL and simply requests the URL with the query args: - `a`: the uri for the content of the start version - `b`: the uri for the content of the end version

It turns out HTTParty has a pretty bad bug with response encodings: jnunemaker/httparty#542 This is hopefully just a temporary patch.

Make Rubocop happy. This syntax is only meaningfully different when modules are used with classes, which we aren't doing. For our use, they're equivalent.

Mr0grog · 2017-06-01T07:08:35Z

Merging so we’ve got something for Global Sprint. Once we’ve got more diff services, we can come back around and consider iterating through DIFFER_SIMPLE_* to auto-create differ instances.

Mr0grog force-pushed the 8-diff-api branch from 7580500 to ac34301 Compare May 31, 2017 08:07

Mr0grog requested review from danielballan and lightandluck May 31, 2017 08:12

danielballan reviewed May 31, 2017

View reviewed changes

danielballan approved these changes May 31, 2017

View reviewed changes

Mr0grog added 5 commits May 31, 2017 23:56

Rough cut at diff pass-through API

8a0e922

Similar route to annotations, since both are about a change between two versions: GET /api/v0/page/{page id}/changes/{from id}..{to id}/diff/{diff type}

Add tests for diffing

9645026

Patch HTTParty bug for correct response encoding

2a2aeb4

It turns out HTTParty has a pretty bad bug with response encodings: jnunemaker/httparty#542 This is hopefully just a temporary patch.

Switch module attrs from class to instance style

7517eaf

Make Rubocop happy. This syntax is only meaningfully different when modules are used with classes, which we aren't doing. For our use, they're equivalent.

Mr0grog force-pushed the 8-diff-api branch from ac34301 to 7517eaf Compare June 1, 2017 06:56

Mr0grog merged commit 759fd8f into master Jun 1, 2017

Mr0grog deleted the 8-diff-api branch June 1, 2017 07:08

This was referenced Jun 1, 2017

Add diff API to documentation #64

Closed

Display diffs on page details view edgi-govdata-archiving/web-monitoring-ui#60

Closed

Errors when attempting to view diffs edgi-govdata-archiving/web-monitoring-ui#68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API for getting diffs #60

Add API for getting diffs #60

Mr0grog commented May 31, 2017 •

edited

Loading

Mr0grog commented May 31, 2017

Mr0grog commented May 31, 2017 •

edited

Loading

Mr0grog commented May 31, 2017

danielballan May 31, 2017

Mr0grog May 31, 2017

danielballan left a comment

Mr0grog commented Jun 1, 2017

Add API for getting diffs #60

Add API for getting diffs #60

Conversation

Mr0grog commented May 31, 2017 • edited Loading

Mr0grog commented May 31, 2017

Mr0grog commented May 31, 2017 • edited Loading

Mr0grog commented May 31, 2017

danielballan May 31, 2017

Choose a reason for hiding this comment

Mr0grog May 31, 2017

Choose a reason for hiding this comment

danielballan left a comment

Choose a reason for hiding this comment

Mr0grog commented Jun 1, 2017

Mr0grog commented May 31, 2017 •

edited

Loading

Mr0grog commented May 31, 2017 •

edited

Loading