Web of Science queries #223

dazza-codes · 2017-09-27T23:22:56Z

Fix #197

Provides query functionality for all the new WoS SOAP API endpoints

TODO:

rebase on master after Savon WSDL client for WOS SOAP API #222 merged
parse the publication results
add specs
squash commits

Related issues:

Analysis of work required to use new ScienceWire API #171

Integration test

Use bundle exec rails c and, when you have the WoS credentials in place, run

wos_queries = WosQueries.new(WosClient.new(Settings.WOS.AUTH_CODE, Settings.WOS.LOG_LEVEL.to_sym))
records = wos_queries.search_by_name('Altman, Russ')
records.count #=> 464 (matched the WoS total records available)
records.uids # an array of the UID values
records.print # long listing of all the records XML

atz · 2017-09-29T20:09:21Z

lib/wos_queries.rb

+class WosQueries
+
+  # Default database 'WOK' is an umbrella for everything
+  DATABASE = 'WOK'.freeze


Don't need a constant here, just put the string as the default in initialize. That makes it part of the YARD.

atz · 2017-09-29T20:09:57Z

lib/wos_queries.rb

+  attr_reader :wos_client
+
+  # @param wos_client [WosClient] a Web Of Science client
+  # @param database [String] a WOS database identifier (default 'WOS')


default now WOK

dazza-codes · 2017-10-03T05:44:03Z

I'm stuck trying to work around what seems to be a bug in Nokogiri, reported at

dazza-codes · 2017-10-03T17:50:41Z

I've worked around the Nokogiri issues related to immutability, by using the Document.dup deep copy method.

atz · 2017-10-04T22:41:13Z

lib/wos_queries.rb

+    # @return [Hash] citingArticles parameters
+    def citing_articles_params(uid)
+      {
+        databaseId: @database,


Probably should have an attr_reader :database and use it in these methods instead of referencing the instance variable.

atz · 2017-10-04T22:46:42Z

lib/wos_queries.rb

+
+    # @param uids [Array<String>] a list of WOS UIDs
+    # @return [Hash] retrieveById parameters
+    def retrieve_by_id_params(uids)


A ton of repetition here in the params methods. Would it make sense to just have a base_params method (that looks exactly like the body of this one) and then invoke:

base_params.merge(different_key: different_val)

in the places where individual key/val pairs need to be added or overridden? This would put the focus on what is unique to each set of params rather than repeating all the things that are in common.

If you look closely at it, there are slight differences in each set of params. Given that we know the request XML is order-sensitive (right? why? weird, if so, but yea ...), doesn't this make it difficult to use a Hash.merge strategy with any confidence? I think we should be glad that savon allows us to use Hash params. These methods have already been refactored a bit to allow for some existing use-case flexibility; we might discover more as we go. I'm open to exploring whether or not a Hash.merge strategy might work and will try it out soon. If it's not a blocker right now, maybe we can move on and adapt as we go?

Yes, I acknowledged those slight differences in my comment. My intent is to have a common starting point, then merge/reject from there. This would make it obvious what each of those differences is, I.E., what is special about this method.

The ordered XML thing does not apply since we are still giving the Savon gem a Hash. It figures out the order now, and it would figure out the order the same way, since we would be passing it exactly the same param. This is just about how we build the hash and how much we repeat ourselves. In general, this codebase has suffered from lack of facility manipulating ruby Hash, so I would like to start repairing that.

I'll take another look at this.

atz

Some comments and a couple requests.

atz · 2017-10-04T23:13:46Z

spec/lib/wos_records_spec.rb

+  describe '#to_xml' do
+    it 'works with encoded records' do
+      result = wos_records_encoded.to_xml
+      expect(result).to be_an String


Can we get a stronger expectation here than just "gimme a String"? Since we are dealing with encoding, it is sorta important that not all strings are equivalent.

Maybe an XSD validation for the record data?

Or at least expect it not to contain < or something.

I don't think full XSD is required. Just look for something that you expect the encoding to adjust (or not adjust) and target it.

atz · 2017-10-04T23:14:04Z

spec/lib/wos_records_spec.rb

+  describe '#decode_records' do
+    it 'works' do
+      result = wos_records_encoded.send(:decode_records)
+      expect(result).to be_an String


atz · 2017-10-04T23:35:01Z

lib/wos_records.rb

+    unless uid_intersection.empty?
+      # create a new set of records, use a philosophy of immutability
+      # Nokogiri::XML::NodeSet enumerable methods do not return new objects
+      # Nokogiri::XML::Document.dup is a deep copy


Note: the hangup with Nokogiri is unsurprising and doesn't require a bunch of commentary. If you want to build a nodeset from existing nodes without retaining any connection back to the parent document (which all nodes have), then you can round-trip the node through serialization .to_xml and back into a new Nokogiri XML node/fragment. This would reduce the memory utilization by not copying the whole document.

Also, it is not a bug (in Nokogiri).

We can discuss this in person, if necessary, as I like it as it is. I hope it's not a blocker. The round trip via .to_xml will not gain anything, as the doc only contains records anyway. The explanation fills a gap in the Nokogiri documentation and I believe will serve as a note to prevent later refactoring that could easily mistakenly introduce violations of the immutability desired (which should be caught by specs, but might not be understood without this commentary).

The round trip is what divorces it from the parent document (since it becomes just a String value, with no ability to point to location of the originating document in Ruby memory). The separation is the basis of your motivation to .dup. I agree this may benefit from better docs, but it is not unexpected behavior.

I get how the .to_xml works. What I'm saying is that the doc just contains REC elements, so there's nothing else duped by the doc.dup anyway, so there's not much gained from all the extra serialization transforms involved in the .to_xml; it will just slow things down and that might be worse that a faster doc.dup when collecting 100's of records.

I agree there may be a computational tradeoff for the benefit of smaller memory footprint. We can defer on this one for later.

atz · 2017-10-04T23:43:09Z

lib/wos_queries.rb

+
+  # @param name [String] a CSV name pattern: {last name}, {first_name} [{middle_name} | {middle initial}]
+  # @return [WosRecords]
+  def search_by_name(name)


Why do you have these 5 public methods that are effectively aliases for private ones? Put the body of the private methods here and remove the "collator" methods.

I'll try to move some stuff where this is done in #242 already, into this PR. This pattern of using a collator emerged from doing one request first and then another and another and then noticing that the collators themselves could be refactored, until they became so small that they, well, don't need to exist any more.

atz · 2017-10-05T00:29:41Z

lib/wos_records.rb

+  # @param record_setB [WosRecords]
+  # @return [Set] duplicate WoS UID values
+  def duplicate_uids(record_setB)
+    uids.to_set.intersection(record_setB.uids.to_set)


Array already has an intersection operator in Ruby, the &. So without the need to convert to_set, this should be uids & record_setB.uids. Intersection also deduplicates multiples and preserves order!

OK, I'll try that, good tip.

Yep, this works for me:

> x = [*1..10] => [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] > y = [*1..10].reject {|i| i % 2 == 0 }.reverse => [9, 7, 5, 3, 1] > x & y => [1, 3, 5, 7, 9] > x.reverse & y => [9, 7, 5, 3, 1] > x.reverse & y.reverse => [9, 7, 5, 3, 1]

dazza-codes · 2017-10-05T03:59:48Z

Review changes pushed in separate commits to clearly identify how each commit addressed the review concerns.

Most of the existing review concerns are fixed, let's review again.

atz · 2017-10-05T21:02:29Z

spec/lib/wos_records_spec.rb

+  describe 'Enumerable mixin' do
+    it 'responds to Enumerable.instance_methods' do
+      methods = Enumerable.instance_methods
+      methods.each { |method| expect(wos_records_encoded).to respond_to(method) }


There is no need to enumerate methods individually, because that is how Ruby inheritance works. Please replace this spec with described_class.is_a?(Enumerable) or consider removing it.

atz · 2017-10-05T21:07:50Z

Thanks for the responses.

The .dup and Hash composition comments are not blockers, as long as you are open to their revision in the future. In that regard, I would as: are you comfortable enough with the tests coverage to permit that?

Would appreciate attention to one outstanding item re: Enumerable testing.

Accommodated review

dazza-codes added the in progress label Sep 27, 2017

dazza-codes mentioned this pull request Sep 27, 2017

Savon WSDL client for WOS SOAP API #222

Merged

1 task

dazza-codes force-pushed the wos-queries branch 5 times, most recently from b89be30 to f35ade1 Compare September 29, 2017 19:56

atz reviewed Sep 29, 2017

View reviewed changes

dazza-codes force-pushed the wos-queries branch 16 times, most recently from e3bd992 to d8876a1 Compare October 3, 2017 00:39

dazza-codes force-pushed the wos-queries branch 2 times, most recently from 3678a3b to 2398dbd Compare October 3, 2017 17:18

dazza-codes force-pushed the wos-queries branch from ecb83f6 to ee4a4d6 Compare October 3, 2017 18:56

dazza-codes changed the title ~~[WIP] WoS queries~~ Web of Science queries Oct 3, 2017

dazza-codes added 3 commits October 3, 2017 20:52

Add htmlentitites gem to decode WOS records

4212aff

WosRecords - utils for WOS records

ccf56cf

WosQueries - Web of Science queries

a9d8375

dazza-codes force-pushed the wos-queries branch from 3161344 to a9d8375 Compare October 4, 2017 04:04

This was referenced Oct 4, 2017

Confirm the new WoS API meets the minimum requirements for our current use cases #231

Closed

WoS DOI Search #242

Merged

atz reviewed Oct 4, 2017

View reviewed changes

atz suggested changes Oct 4, 2017

View reviewed changes

atz reviewed Oct 4, 2017

View reviewed changes

atz suggested changes Oct 4, 2017

View reviewed changes

atz previously requested changes Oct 5, 2017

View reviewed changes

dazza-codes added 4 commits October 4, 2017 17:59

WosQueries - refactor to remove private collator methods

1edf337

WosQueries - attr_reader for database

526f7c5

WosRecords - improve XML encoding specs

1513d2d

WosRecords - use Array intersection for duplicate_uids

4576912

Update .rubocop_todo.yml

0097a62

atz previously requested changes Oct 5, 2017

View reviewed changes

dazza-codes added 2 commits October 5, 2017 14:55

WosRecords - instance is_a? Enumerable

90889e2

WosQueries - refactor UID query params to use a base_uid_params hash

b2702a7

dazza-codes force-pushed the wos-queries branch from dbae5f2 to b2702a7 Compare October 5, 2017 22:16

atz merged commit febbefb into master Oct 5, 2017

atz removed the in progress label Oct 5, 2017

atz deleted the wos-queries branch October 5, 2017 22:57

dazza-codes self-assigned this Oct 6, 2017

Web of Science queries #223

Web of Science queries #223

Conversation

dazza-codes commented Sep 27, 2017 • edited Loading

Integration test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dazza-codes commented Oct 3, 2017 • edited Loading

dazza-codes commented Oct 3, 2017

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

atz 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

atz Oct 4, 2017 • 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

dazza-codes Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dazza-codes commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atz commented Oct 5, 2017

dazza-codes commented Sep 27, 2017 •

edited

Loading

dazza-codes commented Oct 3, 2017 •

edited

Loading

atz Oct 4, 2017 •

edited

Loading

dazza-codes Oct 5, 2017 •

edited

Loading