loyaltynz / hoodoo Goto Github PK

Since the (possibly unwise) introduction of on-by-default framework search for created_by there's been a chance of a 500 being raised for a service which doesn't have the relevant column and doesn't opt out of that search either.

It's also a little tedious to hand-type in the implementation, interface, resource, model and comprised_of declaration updates for a given resource-model pairing by hand.

A Rails-like scaffold generator could help a lot here. Tracking this under Hoodoo and under the service shell.

Moving from 1.14.0 -> 1.15.0 seems to break some previous functionality from Hoodoo::Services::Session as far as I can tell this code:

if Hoodoo::Services::Middleware.environment.test? && MockDalliClient.bypass? == false
  return MockDalliClient.new
end

was removed see git diff v1.14.0..v1.15.0 lib/hoodoo/services/services/session.rb the impact being that previously when in a test environment you could give nothing to the Hoodoo:Services:Session initialiser for memcached_host allowing you to run tests without Memcached installed.

The replacing functionality returns exceptions raised from the new method in the same class

def get_store
...
raise "Hoodoo::Services::Session\#get_store: Cannot connect to Memcached at '#{ host }': #{ exception.to_s }"

This is technically a breaking change but only really affects tests, and I am not sure if the functionality needs to be put back but we should at least make it known in the CHANGELOG?

Hoodoo understands "expected duplications" via X-Deja-Vu: yes in a request (or endpoint.deja_vu = true if using Hoodoo::Client) but only signals that a duplicate violation exception was returned by the database engine via X-Deja-Vu: confirmed / result.response_options[ 'deja_vu' ] == 'confirmed'.

Very often, callers will want to recover the original resource. Right now the best they can do is hope that the destination resource offers a lookup ability based on the duplicate value - typically a search/filter option in #list - but this opens up extra error conditions and complexity in the API calling code as well as bulking out the API of the destination dupication-detecting resource.

Some options are:

• Return it as the response inline, which is kind of what you'd expect, but it might break clients that simply check for an empty response rather than 204 status code or X-Deja-Vu: confirmed header.
• Return the ID of the item as (say) an additional header, e.g. X-Deja-Vu-Item-ID: <uuid>, which still requires an additional call to look up the data but does mean it's a simple #show.

#223 introduces acquire_in, which adds errors to the given context automatically. This arises from the addition of generic.contemporary_exists, which would cause quite high and error-prone service implementation complexity if no automated approach was provided.

Although create-in/update-in/delete-in context mechanisms don't necessarily have the same level of complexity, there is still common boilerplate arising. For those who don't like the idea of mutating the context hierarchy then that approach is still fine, but adding in corresponding "!" methods makes sense for those who want simpler service code.

Examples:

• update_in leads to update_in! (could add both "not-found" and e.g. validation errors)
• manually_dated_destruction_in leads to manually_dated_destruction_in! (could add "not-found")

When Airbrake errors are reported in a Rails application details of the request and environment are reported to help with diagnosing the problem. It would be good to change Hoodoo to report details from the context like some of the session and request when reporting exceptions.

[Joseph Leniston]

Make this a first-class module which can be used in its own right, rather than something embedded into the middleware.

Technically it can be used on its own, but the namespacing is weird. It'd break Guides links and the like to move it completely, so this would be fiddly. It could be OK just to leave it as-is, but it's messy.

[Andrew Hodgkinson]

PostgreSQL's fast count estimation tends to be very inaccurate a lot of the time, but if we assume it might be at least of some vague use on large dataset sizes, we should at least do something for small ones - if the estimated size is less than the actual list count of a returned chunk of data, Hoodoo ought to rewrite it to match the actual known size.

Low priority / nice-to-have.

Consider:

  class TestHashKeyDeepDefaultAggregation < Hoodoo::Presenters::Base
    schema do
      hash :test do # A default here would implicitly override anything on :two below

        key :one,   :default => { :foo => 'bar' }
        key :three, :type    => :integer

        key :two do
          hash :inner_two, :default => { 'inner_three' => 'three' } do
            key :inner_one,   :default => { :bar => 'baz' }
            key :inner_three, :type    => :text
          end
        end
      end
    end
  end

...this might seem reasonable:

  it 'aggregates default deep Hash and key values' do
    expected = {
      'test' => {
        'one'   => { 'foo' => 'bar' },
        'two'   => {
          'inner_two' => {
            'inner_one'   => { 'bar' => 'baz' },
            'inner_three' => 'three'
          }
        }
      }
    }

    expect( TestHashKeyDeepDefaultAggregation.render( {} ) ).to eql( expected )
  end

...but the presenter engine cannot "see" the Hash default on key :two, because the embedded block runs in a context that's not aware of its parent and thus when the rendering code is stepping through the parent Hash's properties, it sees no data for :two and no default on that specific property, even though there's a "hidden"/unnoticed default sitting inside which logically a coder might assume would work: https://github.com/LoyaltyNZ/hoodoo/blob/master/lib/hoodoo/presenters/types/hash.rb#L300 (there is no key in the inbound data but the local property has no defaults either, so the loop hits the next condition and does not render anything).

This is an arguable limitation of the DSL but could be considered a bug, so it should either be documented or somehow fixed (the mess of trying to have children address their parent's default properties is probably too nasty to contemplate and pushes us towards The Rewrite Mistake on the presentation and validation engine).

A work-around is to put a nested Hash default up a level, onto the key, but this is not obvious and inelegant as one must then remember to include the inner Hash's name as a key in the outermost part of the default thus:

        key :two, :default => { 'inner_two' => { 'inner_three' => 'three' } } do
          hash :inner_two do

This line

Adding a comment, and a test to ensure this behaviour is a requirement of saving a session would be useful to make sure that this line is never optimised out of the code.

The interaction id for a request/response is available via the response_options Hash in Hoodoo::Client::AugmentedBase. When a call is successful the interaction id is populated correctly, when an error response is received the interaction id is nil. Example to reproduce:

irb> client.resource( :NotARealResource ).show( 'foo' ).response_options[ 'interaction_id' ]
=> nil
irb> client.resource( :NotARealResource ).list.response_options[ 'interaction_id' ]
=> nil

Although they're an anti-pattern, they're also official - a service implementation can implement methods #before or #after (passed context as with the usual action methods like #list or #create) and have these called around their action method dispatch by Hoodoo middleware.

• https://github.com/LoyaltyNZ/hoodoo/blob/master/lib/hoodoo/services/middleware/middleware.rb#L1911

This won't appear in RDoc as it isn't something Hoodoo implements, it's something a service can implement.

Add to Hoodoo Guides.

Sort of related to #255 - Hoodoo framework search/filter additions automatically validate the header quantities, so things like a created_by search is rejected with a reasonably informative 422 response should the search field's value not be a valid UUID.

Currently, there's no way for a resource interface class to request something similar. Models can provide quite complex mappings to get from search/filter request down to SQL level, while interfaces can give a basic list of things that are searchable or filterable, but there's no easy place to have validation and error returns done other than hand-coding it at Some Appropriate Point in the implementation flow.

This could be improved upon dramatically.

Hoodoo::Services::Middleware::DEFAULT_TEST_SESSION is injected into the context.session of public actions. Public actions do not have a session so context.session should return nil.

This means that it's possible to write code which interacts with the session (i.e.context.session.scoping) which works in development and test environments but doesn't work in deployed environments.

The bug is in Hoodoo::Services::Middleware#load_session_into where the check is only:

elsif ( self.class.environment.test? || self.class.environment.development? )

It needs to also check that the action is non-public.

I've had a quick look into how this could be done. The Hoodoo::Services::Middleware#process method determines the interface_class but in a rather complex manner. Once the interface_class is found, it should be straight-forward to find the defined public_actions and use that in the above elseif statement

Come up with a clean, legible, but no-per-request-overhead way to pretty print vs terse print JSON in development mode versus other modes.

Has same questions associated as #107 and might well be implemented at the same time.

[Andrew Hodgkinson]

In Hoodoo::Services::Response:

• https://github.com/LoyaltyNZ/hoodoo/blob/master/lib/hoodoo/services/services/response.rb

...there was originally just a #body accessor used to set response data, but this was later improved with some higher-level names of #set_resource and #set_resources. These were originally implemented just as pure aliases (and #set_resource still is), but the introduction of "estimated dataset size" meant that #set_resources and #set_estimated_resources ended up implemented as small, bespoke methods.

• https://github.com/LoyaltyNZ/hoodoo/blob/master/lib/hoodoo/services/services/response.rb#L143
• https://github.com/LoyaltyNZ/hoodoo/blob/master/lib/hoodoo/services/services/response.rb#L180

This is handy because Hoodoo really ought to check that an Array-like object is passed here. The rendering behaviour in terms of the top-level _data key and Array value (or not) is switched not based on the inbound API request method, of which the Response class is unaware, but simply on the data type passed. This makes it easy for a service author to accidentally pass a "singular" resource (e.g. a Hash) in a #list implementation under certain circumstances which might lead to an implementation that "knows" it'll only return a single instance.

A simple Type check would solve the problem. Since the switching behaviour is here:

• https://github.com/LoyaltyNZ/hoodoo/blob/master/lib/hoodoo/services/services/response.rb#L340

...then the same type check semantics ought to be used in #set_resources and #set_estimated_resources, so it makes sense to extract the check into a private method and call from all three places.

• Extract Array check from #for_rack into private method
• Add call to this into #set_resources and #set_estimated_resources raising an exception if a bad type is encountered
• Add tests to ensure 100% non-trivial coverage on updates
• Update documentation if necessary, including checking Hoodoo Guides

For each inbound event, two log events are generated where code=inbound and level=info but only one of these events contains information like body or participant id. An easy way to find these "bad" log events is to search for "participant_id=null" in log entries.

I am not sure if these events are deliberate or accidental, but the "bad" event has little information to make it useful.

The introduction of created_by as an on-by-default framework-level search filed opened up services to 500 errors when Hoodoo's finder code automatically added a created_by column search if so requested by the URI query string, but that column didn't exist (uncaught database exception typically arises, propagating up out of ActiveRecord).

Investigate ways to handle this more elegantly - e.g. detect the specific not-found-column exception case for framework related search columns and translate to a 422 instead.

We are re-mapping 404 errors to 422 errors like this:
https://github.com/LoyaltyNZ/service_purchase/commit/c86df271e26ce16c36719018579062f207e32b89#diff-ca30485c7902870cefae80e188330bebR147
called here:
https://github.com/LoyaltyNZ/service_purchase/commit/c86df271e26ce16c36719018579062f207e32b89#diff-ca30485c7902870cefae80e188330bebL33

Which in english is basically changing a Hoodoo::Errors instance inline, which works fine except there is no public api to change the http_status_code for a Hoodoo::Errors instance. I had to resort to resource.platform_errors.instance_variable_set(:@http_status_code, 422) to change the status code from a 404 to a 422.

So a couple ideas I had for Hoodoo are make @http_status_code an :attr_accessor, instead of just an :attr_reader, or create a new public method on Hoodoo::Error that "refreshed" the http_status_code based on the current errors. Thoughts?

Since created_at is a fundamental resource property and is used as a default sort column for all resources unless explicitly overridden, it makes sense to have default search/filter stuff built in via the ActiveRecord extensions. Concerns:

• Doing this cleanly - one attempt ended up making a mess, because of trying to shield the service application from automatically handled parameters in part
• Burdening service authors - if this is a default, then service authors / resource documentation has to explicitly state when it is not supported, and / or go to extra pains on non-ActiveRecord systems or with more difficult model arrangements to support it.

[Andrew Hodgkinson]

example:

        schema do
          string  :currency_code, :required => true, :length => 5
          integer :precision, :default => 2

        end

and a POST to create a new instance with

{
   "currency_code": "NZD"
}

will fail:

          response.errors.add_error(
            'generic.invalid_parameters',
            'message' => 'Body data contains unrecognised or prohibited fields'
          ) if body != verification_object.render( body )

because verification_object.render( body ) will render:

{
   "currency_code": "NZD",
   "precision": 2
}

Older code using an HTTP(S) proxy has to do stuff like this to instantiate a Hoodoo::Client:

options = {
  :base_uri => SomeName::Application.config.x.platform_api_endpoint,
  # ...other things...
}

unless Rails.env.test?
  options[ :discoverer ] = Hoodoo::Services::Discovery::ByConvention.new( {
    :base_uri  => SomeName::Application.config.x.platform_api_endpoint,
    :proxy_uri => ENV[ 'https_proxy' ]
  } )
end

return Hoodoo::Client.new( options )

Can that be automated? Doesn't Net::HTTP self-configure?

Hoodoo doesn't yet offer any support at all for "_embed"/"_reference" implementations. For services wishing to enforce permissions, inter-resource call permissions augmentation makes life difficult - a "several hops downstream" resource endpoint may implement some embed behaviour, check its session and find that appropriate permissions exist, but won't realise that (perhaps) these are only there as part of permissions addition requested by a resource interface earlier in the chain.

Really, the only way this can be solved is through Hoodoo additions/changes; the general principle is that Hoodoo handles as much authorisation/authentication/permissions stuff as possible to minimise service author burden and minimise the chances of service oversight/omission, so really Hoodoo ought to significantly extend its support for embedding and auto-enforce permissions as far as possible.

(Intentional duplicate of #43).

Hoodoo v1.1 introduced a no-convention routing mechanism where /1/Foo sits alongside /v1/foos. There were some API additions alongside this. RDoc is up to date; Hoodoo Guides need to be updated.

PR #229 introduces a test dependency on WebMock for expediency, but only uses it for this one test alone. It's a heavy dependency to pull in for that one use case; consider instead manually mocking out just the Net::HTTP bits of interest for those tests to slim back down the Gemfile.lock a little.

Uses the Transient Store backend once created (see #105) assuming a single shared instance of that store. Knows when someone creates, updates, deletes a resource and thus knows how it's represented for a simple show, or a simple list etc., or when it should invalidate such thing. Idea is to avoid in- or inter-resource calls by having the result already in the cache.

Depends upon #105.

[Andrew Hodgkinson]

When using .list(context.request.list and the limit is not validated in the service, it is passed to Hoodoo, which results in a postgres error:

(Part of error omitted for sake of sanity)

"errors": [
    {
      "code": "platform.fault",
      "message": "PG::InvalidRowCountInLimitClause: ERROR:  LIMIT must not be negative\n: SELECT  DISTINCT tokens.* FROM \"tokens\" INNER JOIN memberships ON memberships.token_identifier = tokens.identifier WHERE \"tokens\".\"deleted_at\" IS NULL AND (memberships.deleted_at IS NULL AND memberships.programme_code IN ('FB'))  ORDER BY \"tokens\".\"created_at\" DESC LIMIT -1 OFFSET 0",
      "reference": "PG::InvalidRowCountInLimitClause: ERROR: <SNIP>"
    }
]

As title. When working on a hotfix for v2 I tried upgrading the Rack patch to a Hoodoo class instance monkey so that the aliases for the original Rack start could go away and we'd just use super, but it was never called so couldn't really be tested - but then I found I couldn't get the original Rack patch to get invoked either. Rack::Server.start isn't called at all, or is called before the monkey code is parsed.

Investigate and resolve. Either the patch is unnecessary now or it should be upgraded to a real Hoodoo monkey (and loaded implicitly after all other requires by the main monkey system, rather than by the middleware.rb requirement script).

starting a service with

AMQ_URI="amqp://localhost" AMQ_ENDPOINT="service.authentication" rackup -s alchemy

instead of

MEMCACHED_HOST="localhost:11211" AMQ_URI="amqp://localhost" AMQ_ENDPOINT="service.authentication" rackup -s alchemy

causes an error when being called with stack

E, [2015-02-18T15:06:04.953720 #30694] ERROR -- : undefined method `to_msgpack' for #<RuntimeError:0x007f95743dfcf0> (NoMethodError)
/Users/grahamj/.rvm/gems/ruby-2.1.5/bundler/gems/amq-endpoint-03296a065775/lib/amq-endpoint/message.rb:53:in `pack'
/Users/grahamj/.rvm/gems/ruby-2.1.5/bundler/gems/amq-endpoint-03296a065775/lib/amq-endpoint/message.rb:53:in `serialize'
/Users/grahamj/.rvm/gems/ruby-2.1.5/bundler/gems/hoodoo-ec56abf72827/lib/hoodoo/services/middleware/amqp_log_message.rb:120:in `serialize'
/Users/grahamj/.rvm/gems/ruby-2.1.5/bundler/gems/amq-endpoint-03296a065775/lib/amq-endpoint/endpoint.rb:195:in `block (2 levels) in create_tx_thread'
/Users/grahamj/.rvm/gems/ruby-2.1.5/bundler/gems/amq-endpoint-03296a065775/lib/amq-endpoint/endpoint.rb:184:in `loop'
/Users/grahamj/.rvm/gems/ruby-2.1.5/bundler/gems/amq-endpoint-03296a065775/lib/amq-endpoint/endpoint.rb:184:in `block in create_tx_thread'

When Hoodoo is receiving a request into a known set of interface declarations, then if using a test session, only the interfaces' declared resources and supported actions of those resources ought to be permitted within the session.

Going back to the earliest days of its development, for expediency the test session had a global "else allow" on all resources. This is fine until inter-resource calls start to happen, at which point if additional_permissions_for is not declared, a test might pass but a real-world deployment would fail.

Mocking/stubbing may defeat this regardless, but at least there'd be a fighting chance of spotting such issues under test if there was a less permissive approach to the test session permissions.

Related: #101

As title. Is this worth doing, or leave it up to service authors to implement? Doing it without overhead will be tricky - it'll need an environment check, but maybe there's already one in the processing chain at a point we can use it (if not, it might be time to create that pathway).

See als #108, which might be implemented at the same time.

Currently in Hoodoo::Services::Discovery::ByConvention, active_support/inflector is required but there is also a rescue LoadError in this class. This means that if active_support is not installed then this file will fail to load but without an error message. Then when attempting to use the Hoodoo client in its default state:

client = Hoodoo::Client.new(base_uri: 'https://api.loyaltyblue.co.nz', caller_id: c_id, caller_secret: c_s)

...one is returned the confusing error:

uninitialized constant Hoodoo::Services::Discovery::ByConvention

Would be awesome if this situation would hint at the fact that we are just missing active support.

[David Mitchell]

(Intentional duplicate of #94).

DSL for embeds - make it accept a Hash where a better structure is provided, e.g. with :comment for docs?

[Andrew Hodgkinson]

At the moment to group the "inbound" log events by resource (to find out usage rates) I have to use regex on the path attribute.

If we add the resource to the inbound data hash then grouping would be easier.

Formalise a simpler way for service authors to create Mock Resources for testing calling remote resources. See the Testing Guide for the current approach. The correct for this might be either here (Hoodoo::Testing...?) or the service shell - not sure.

[Andrew Hodgkinson]

See also: #245

When passing non-string values to the render method this causes an Exception to be raised during serialisation (amqp_log_message.rb:148 the call to super).

The exception is undefined method 'to_msgpack' for 2015-07-27 01:41:36 UTC:Time (NoMethodError)

What happens is the service dies and the caller is returned a 408 (timeout) response.

I would expect this not to kill the service and return a 500 error instead.

example reported_at from logentries datetime is "2015-06-07 19:31:33.180705910000 UTC" which is not the standard the rest of the platform uses.

It's a common idiom to write code such as:

foo_endpoint = some_caller.resource( :Foo )

The method name half works and half doesn't; you're getting a proxy object to the resource, but you're not getting a resource itself, just the means to manipulate them. It is, indeed, more of an endpoint. So - to allow for a more natural coding style for people who think "that way around", alias the method name to #endpoint, allowing either to be used.

Create a new namespace class for transient storage, with at least one plug-in mechanism for Memcached. Refactor the Sessions system to use this instead of a hard dependency on Memcached or the test session.

Question: What's the best name? TransientStore is clumsy.

See also: #106

[Andrew Hodgkinson]

Hoodoo doesn't yet offer any support at all for "_embed"/"_reference" implementations. For services wishing to enforce permissions, inter-resource call permissions augmentation makes life difficult - a "several hops downstream" resource endpoint may implement some embed behaviour, check its session and find that appropriate permissions exist, but won't realise that (perhaps) these are only there as part of permissions addition requested by a resource interface earlier in the chain.

Really, the only way this can be solved is through Hoodoo additions/changes; the general principle is that Hoodoo handles as much authorisation/authentication/permissions stuff as possible to minimise service author burden and minimise the chances of service oversight/omission, so really Hoodoo ought to significantly extend its support for embedding and auto-enforce permissions as far as possible.

When reporting logs via Hoodoo::Logger::StreamWriter, log entries which are in the process of being written to file sometimes do not make it to the file if the main process completes too fast.

This primarily affects any services using Hoodoo::Client outside of the loyalty platform - such as the flybuys website making calls to the loyalty platform. In the http endpoint http.verify_mode = OpenSSL::SSL::VERIFY_NONE disables SSL cert chain verification, which means that any valid SSL cert, even if it's not actually ours will be accepted. This opens us up to a man in the middle attack.

The primary reason for this verification being disabled, appears to be for the purposes of local development / testing. A potential fix might be to only use this downgrade setting if the RACK_ENV is set to development or test - though this will need confirming.

Currently in Hoodoo::Services::Discovery::ByConvention, active_support/inflector is required but there is also a rescue LoadError in this class. This means that if active_support is not installed then this file will fail to load but without an error message. Then when attempting to use the Hoodoo client in its default state:

client = Hoodoo::Client.new(base_uri: 'https://api.loyaltyblue.co.nz', caller_id: c_id, caller_secret: c_s)

...one is returned the confusing error:

uninitialized constant Hoodoo::Services::Discovery::ByConvention

Would be awesome if this situation would hint at the fact that we are just missing active support.

[David Mitchell]

Perhaps e.g. go with return ... and the returned item is either auto-rendered, or is the rendering of the resource, but either way no explicit context.response.set_resource. It's implicitly singular for any action other than list, else plural. Declarative access to estimation counts, but since estimation counts seem to be basically useless maybe drop that entirely.

Just a placeholder for some deeper thinking about whether or not we break the API's backwards compatibility in V2 deeply, because it's really worth it in terms of reducing boilerplate with 3 years of hindsight now.

Sometimes very small clock drifts between nodes have the effect of rejecting a valid call because the default to set X-Dated-At header as Time.now (if none specified) can be very slightly ahead of the node it's sending the header to which then returns as invalid because the datetime can't be in the future. Would be good to allow a few seconds in the future to allow for these minor clock drifts.

Extend the resource interface DSL to support resource name aliases.

This is useful for cases where, as time goes by, it becomes obvious that a particular resource is part of a growing family with a particular naming strategy but one historically named item is inconsistent, making things less easy to follow/understand in documentation and just generally more ugly. To avoid breaking historic callers the resource should forever be addressable by its original name(s), while also renaming it to a new recommended form.

Given that we have to read the callers key to authenticate a session, it makes more sense to move the caller information into the caller key in memcached. This would have the additional bonus of reducing the size in memcahced because there may be one caller to potentially many sessions.

caller version may still need to be checked, as updating a caller might still warrant the need to invalidate all sessions attached to it.

Query string extension that tells receiver to just return IDs only, not representations, so Hoodoo doesn't have to strip them (but it will for older services that don't implement this).

It could be a question of being able to specify a particular single field you want, with "id" just being the obvious case for fast referencing.

[Andrew Hodgkinson]

I want to use a schema, in two different contexts but have the definition shared. For example I have defined a phone number as:

class PhoneNumber < Hoodoo::Presenters::Base
  schema do
    string :country_code
    string :area_code, :required => true
    string :number, :required => true
  end
 ...
end

and then have it used in the same schema with different prefixes for example:

class Person < Hoodoo::Presenters::Base
  type PhoneNumber, :key_prefix => 'mobile_'
  type PhoneNumber, :key_prefix => 'home_'
  ...

And resulting in data like:

{
  "mobile_country_code": "+64",
  "mobile_area_code": "21",
  "mobile_number": "6453 3232",
  "home_country_code": "+64",
  "home_area_code": "4",
  "home_number": "111 2234",
  ...
}

Hoodoo does filter out body parameters that are not included in the Resource's to_create/to_update. This allows specifying/changing of id's, created_at. Additionally if the body hash is used to update a model, any attribute on the model can be updated (including ones not visible in the presented json).