Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize/dedupe external-request functions,leveraging caching where worthwhile #107

Open
rowanthorpe opened this issue Apr 3, 2018 · 1 comment
Labels
enhancement Improvements to existing bits of code.

Comments

@rowanthorpe
Copy link
Collaborator

rowanthorpe commented Apr 3, 2018

I'm opening this issue to continue the thread started about here on this now-closed pull-request. I won't copy-paste the existing thread here, it is easy enough to click through to read it.

@rowanthorpe rowanthorpe added the enhancement Improvements to existing bits of code. label Apr 3, 2018
@GioBonvi
Copy link
Owner

GioBonvi commented Apr 3, 2018

This is a list of all the "external resources" that the script needs to fetch (through various different calls):

  • External web page
    • Uniquely identified by: URL http://example.com?this=that
    • Fetched via: UrlFetchApp.fetch(URL)
    • Benefits from caching: yes, always
    • Failure:
      • If the HTTP status code of the response is not 2xx (the request has not succeeded) an error is thrown
  • Google Contact object
    • Uniquely identified by: ID http://www.google.com/m8/feeds/contacts/EMAIL/base/CONTACT_ID
    • Fetched via: ContactsApp.getContactById(ID)
    • Benefits from caching: yes, always
    • Failure:
      • If EMAIL does not match the email of the user running the script or if CONTACT_ID does not match any contact ID null is returned
  • Google Plus Profile object
    • Uniquely identified by: ID 21 digit string
    • Fetched via: Plus.People.get(ID)
    • Benefits from caching: yes, always
    • Failure:
      • If ID does not match any profile ID an error is thrown
  • Google Calendar
    • Uniquely identified by: ID string (formatted like an email address)
    • Fetched via: Calendar.Calendars.get(ID)
    • Benefits from caching: not really, but it wouldn't hurt either
    • Failure:
      • If ID does not match any calendar ID an error is thrown

Additionally there are two other "resources" that can cause troubles when "accessed" incorrectly; they are not properly "external resources", but they still need to be dealt with:

  • JSON object
    • Uniquely identified by: nothing
    • Fetched via: JSON.parse(str)
    • Benefits from caching: possibly
    • Failure:
      • If str is not a valid JSON string an error is thrown
  • SimplifiedSemanticVersion
    • Uniquely identified by: a SimplifiedSemanticVersion string
    • Fetched via: new SimplifiedSemanticVersion(str)
    • Benefits from caching: not really
    • Failure:
      • If str is not a valid SimplifiedSemanticVersion string an error is thrown

Lastly, there is Utilities.formatDate(date, timeZone, formatString) which in a previous version could fail throwing an error if the timeZone parameter was invalid, while now it simply deafults timeZone to '' without throwing any error.

Having established this I concur that we need a way to standardize these external calls (and remove all the redundant try/null checks).

I'd propose to:

  • write a function (fetchExternalResource) to perform any external request throwing an error on failure;
  • have LocalCache leverage this function for caching;
  • use directly fetchExternalResource when caching is not needed;
  • not consider JSON and SimplifiedSemanticVersion as external requests, but still have them wrapped in try-catch blocks to manage their eventual failure in the same way we do with the external requests;

Something like this:

// Enum
var extResType = {
  WEBPAGE: 'URL',
  G_CONTACT: 'GCONT',
   ...
};

function fetchExternalResource(resourceIdentifier, type) {
  switch (type) {
    case extResType.G_CONTACT: // returns null on failure
      result = ContactsApp.getContactById(resourceIdentifier);
      if (result === null) { throw Error(); }
      return result;
    case extResType.WEBPAGE// throws error on failure
      return UrlFetchApp.fetch(resourceIdentifier);
    case extResType.XYZ:
      ...
  }
}

LocalCache.prototype.fetch = function (resourceIdentifier, type, retry) {
  // Use fetchExternalResource() instead of UrlFetchApp.fetch().
  uniqueId = type + ':' + resourceIdentifier;
  this.cache[uniqueId] = response;
  return this.cache[uniqueId];
}

LocalCache.prototype.isCached = function (resourceIdentifier, type) {
  uniqueId = type + ':' + resourceIdentifier;
  return !!this.cache[uniqueId];
};

LocalCache.prototype.retrieve= function (resourceIdentifier, type, retry) {
  ...
}

// Needs caching.
try {
  cache.retrieve('http://example.org', extResType.WEBPAGE);
} ...
// Does not need caching.
try {
  fetchExternalResource('http://example.org', extResType.WEBPAGE);
}...
// Not an external request.
try {
  JSON.parse(str);
}...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to existing bits of code.
Projects
None yet
Development

No branches or pull requests

2 participants