solidusjs / solidus

A simple server that generates pages from JSON and Templates
MIT License
28 stars 7 forks source link

Centralized resources #74

Closed Fauntleroy closed 10 years ago

Fauntleroy commented 10 years ago

Lets developers define resources "globally" with a resources.json file. This file has 2 keys: resources and options. resources is an object of resource data that operates exactly like the resources definition inside page configurations. options is an object of resource options that are mixed into existing resource definitions (in resources.json or page configurations). The keys here are strings that match urls with a wildcard (*), and the values are the options which you wish to set (right now that's just headers and auth).

Here's the resources.json I worked with in my example site:

{
    "resources": {
        "profile": {
            "url": "http://staging.proxy.storyteller.io/Fauntleroy/soundcloud/users/fauntlero-von-patton.json"
        },
        "favorites": {
            "url": "http://staging.proxy.storyteller.io/Fauntleroy/soundcloud/users/fauntlero-von-patton/favorites.json"
        }
    },
    "options": {
        "http://staging.proxy.storyteller.io/*": {
            "headers": {
                "Api-Key": "e801cf0a-ee44-42d3-a29f-aff82fd0521b"
            }
        }
    }
}

I'm not really a huge fan of how this has ended up, but it works. I'd love some suggestions, or maybe a different approach altogether...

pushred commented 10 years ago

It'd be cleaner if the object here was more similar to those defined per view in #73 and maybe even redirects.json (as an array). There's still an opportunity to make resource definition less redundant as well. How about this:

{
    "http://staging.proxy.storyteller.io/Fauntleroy": {
        "resources": {
            "profile": "/soundcloud/users/{username}.json",
            "favorites": "/soundcloud/users/{username}/favorites.json"
        },
        "headers": {
            "Api-Key": "e801cf0a-ee44-42d3-a29f-aff82fd0521b"
        }  
    },
    "http://services.sparkart.net/api/v1/consumer": {
        "resources": {
            "events": "/events?tags={username}"
        },
        "qs": {
            "key": "551fca3b-a134-47a3-a34a-fea6ac38af12"
        }
    }
}

The same format makes sense to me within a view. I think defining headers/auth per resource could get way too dense and redundant. What if I needed to specify the same header for 5+ resources?

Not sure if you included support for query strings in #73 but you should. I'd prefer to spell it out as "querystring" but I figured you're trying to maintain parity with Request.

Does this method replace #73? Or are you supporting both methods for backwards compatibility (and optionality?). If both are supported concurrently, how do you manage name conflicts? "Local"-defined resources in a view win?

pushred commented 10 years ago

One more thing: we should really have the ability to source these credentials from environment variables. That will allow us to template the configuration, keep repos public, and generally improve security. Would you prefer another issue for that?

Fauntleroy commented 10 years ago

I'm not sure what you mean by this comment:

What if I needed to specify the same header for 5+ resources?

If you want to do that, you specify the headers in the options key of resources.json, like so:

{
    "options": {
        "http://staging.proxy.storyteller.io/*": {
            "headers": {
                "Api-Key": "e801cf0a-ee44-42d3-a29f-aff82fd0521b"
            }
        }
    }
}

Now any resources that match the http://staging.proxy.storyteller.io/* URL will use those headers, whether it is defined in resources.json or within a view.

There is no "support" for querystrings aside from including them in the URL proper. Right now options are passed directly to Hyperquest as is, and there is no way to configure query strings aside from directly setting them in the URL. This should be easy enough to support, but I'll have to handle it myself.

This doesn't replace in-view resource definitions, it just augments them. Resources defined at this level can be used within pages by name. It's a little bit redundant, but if you define a resource named cats in your resources.json, you can use it in your view like so:

{{!
    "resources": {
        "cats": "cats",
        "other": "http://api.other.io/resource.json"
    }
}}
Fauntleroy commented 10 years ago

Sourcing credentials from the ENV is a wholly separate issue.

pushred commented 10 years ago

This doesn't replace in-view resource definitions, it just augments them. Resources defined at this level can be used within pages by name. It's a little bit redundant, but if you define a resource named cats in your resources.json, you can use it in your view like so

It was definitely unclear how this file relates to per-view definition. What about the array of names that was discussed before? If I want to reference these global resources do I need to do this name association in every view?

What's the value of per-view definition with the introduction of this file?

"The Hyperquest api is a subset of request" from https://github.com/substack/hyperquest#hyperquest

Does that not mean that it inherits request's API, which includes a qs parameter accepting an object of params?

Fauntleroy commented 10 years ago

@pushred took some time to see if qs would work, and for some reason it doesn't. I'm not really sure if hyperquest is actually a subset of request. I'll have to ask substack about it sometime.

pushred commented 10 years ago

Thanks for checking. Just limits the utility of the options outside of Storyteller.io (or other APIs that use headers/basic auth).

Did the alternative object hierarchy/resource URIs (vs URLs) do anything for you?

Fauntleroy commented 10 years ago

I'm not too keen on that hierarchy because it removes the 1-to-1 association of the resources key in the config to the resources key in the context in views. Since we're currently asking developers to keep so much information in their head, I think it's important that we try to keep as many of these kinds of associations intact as possible.

pushred commented 10 years ago

Well again, what is the value of per-view resource definition with the addition of centralized resources?

If there is value then I think there's already a disconnect because of configuration keys like preprocessor and layout. The url and options keys you're proposing are also in that vein. Are those actually available in my context? layout is indeed included though I'm not sure what it's useful for (would be more so without the extension probably). It's also added automatically even if it isn't specified. What appears in the config object and what's in the context are different. Perhaps something more explicit would work better, while also potentially better accommodating #46 (and another undocumented idea — referencing data from resources as custom metadata):

{
    "context": {
        "title": "Superband",
        "description": "My favorite music, upcoming events, and more!",
        "og:image": resources.profile.avatar_url,
        "http://proxy.storyteller.io/Fauntleroy": {
            "resources": {
                "profile": "/soundcloud/users/{username}.json",
                "favorites": "/soundcloud/users/{username}/favorites.json"
            },
            "headers": {
                "Api-Key": "e801cf0a-ee44-42d3-a29f-aff82fd0521b"
            }  
        },
        "http://services.sparkart.net/api/v1/consumer": {
            "resources": {
                "events": "/events?tags={username}"
            },
            "qs": {
                "key": "551fca3b-a134-47a3-a34a-fea6ac38af12"
            }
        }    

    },
    "layout": "/layouts/3-column",
    "preprocessor": "superband"
}

This adds a layer of nesting but explicitly calling out context may help with this mental model we're trying to establish. There's also the nesting due to the base URL wrappers but this to me is more explicit than the wildcard convention which is going to fail some as well. It also increases the visibility of the resource URIs. Base URLs, versioning parameters and other API-wide constants are just needlessly noisy.

Maybe resources don't even really need to be namespaced? Personally I also still favor separating the credentials into a dedicated keychain.json or credentials.json file, that cleans this up even further and makes #75 far easier.

{
    "context": {
        "title": "Superband",
        "description": "My favorite music, upcoming events, and more!",
        "og:image": resources.profile.avatar_url,
        "http://proxy.storyteller.io/Fauntleroy": {
            "profile": "/soundcloud/users/{username}.json",
            "favorites": "/soundcloud/users/{username}/favorites.json"
        },
        "http://services.sparkart.net/api/v1/consumer": {
            "events": "/events?tags={username}"
        }    

    },
    "layout": "/layouts/3-column",
    "preprocessor": "superband"
}

keychain.json

[{
    "hostname": "http://proxy.storyteller.io",
    "headers": {
        "Api-Key": "e801cf0a-ee44-42d3-a29f-aff82fd0521b"
    }
}, {
    "hostname": "http://services.sparkart.net",
    "querystring": {
        "key": "551fca3b-a134-47a3-a34a-fea6ac38af12"
    }
}, {
    "hostname": "https://basecamp.com",
    "basic": {
        "username": "elanehart",
        "password": "temp123"    
    }
}]
Fauntleroy commented 10 years ago

@pushred I've added the ability to set query params like so:

"test8": {
    "url": "https://hipster.sparkart.net/api/v1/resources/poiu/my-resource-8",
    "query": {
        "test": true
    }
},
"test9": {
    "url": "https://hipster.sparkart.net/api/v1/resources/poiu/my-resource-9",
    "query": {
        "test": "{resource_test}"
    }
}
pushred commented 10 years ago

Cool, that should make reading param configs easier too.

Fauntleroy commented 10 years ago

Also, re: "why even have per page resource config when we have a centralized store?": That's mostly because of backwards compatibility. I don't really think there's any reason to keep those config options there otherwise, not off the top of my head at least.

Fauntleroy commented 10 years ago

While I get what you're going for, I think this format would be more confusing for end users:

{
    "context": {
        "title": "Superband",
        "description": "My favorite music, upcoming events, and more!",
        "og:image": resources.profile.avatar_url,
        "http://proxy.storyteller.io/Fauntleroy": {
            "profile": "/soundcloud/users/{username}.json",
            "favorites": "/soundcloud/users/{username}/favorites.json"
        },
        "http://services.sparkart.net/api/v1/consumer": {
            "events": "/events?tags={username}"
        }    

    },
    "layout": "/layouts/3-column",
    "preprocessor": "superband"
}

We can ask the team about it, but I never liked putting URLs into json keys. I'm amenable to creating something like a keychain.json, and removing resource definition in views themselves. This would get rid of the two keys problem in resources.json and we could use an array to include resources defined in resources.json rather than doubling up on keys.

{{!
{
    "resources": ["pictures","posts"]
}
}}
pushred commented 10 years ago

Also, re: "why even have per page resource config when we have a centralized store?": That's mostly because of backwards compatibility. I don't really think there's any reason to keep those config options there otherwise, not off the top of my head at least.

I was thinking that was the reason, doesn't really seem worth keeping this functionality around though only for that. It's what major versions are for, and updating our existing sites wouldn't require that much effort. Now's really the time to make this sort of change. But let's see what @monikahoex and @krackydile have to say, maybe there's some value that we're overlooking.

Aren't you putting URLs in keys for the options object? :neckbeard:

Fauntleroy commented 10 years ago

I am using URLs with wildcards in the options object, and it's one of the reasons I'm not gung-ho about this approach right now. I'll have to see what else I can come up with.

Fauntleroy commented 10 years ago

How about explicitly associating an auth with a resource: https://gist.github.com/Fauntleroy/7593588

pushred commented 10 years ago

Pretty repetitive still, I think it begs the question of the use case of using different credentials for different (or the same endpoint) within the same API.

I can think of one case where we came close: on Young Money the original plan was to have two Tumblr accounts: one for Young Money and one for Lil Wayne. If we'd done that we may have needed separate apps and credentials in order to access drafts for each respective account. But even maintaining 2 separate Tumblr blogs was too much for them in that case so didn't happen. But since it's actually the access token granted based on the account providing auth so long as the account had access to both blogs we wouldn't have needed separate credentials.

Specifying base URLs would still accommodate similar scenarios though if it could include parts of path segments in addition to the host.

Fauntleroy commented 10 years ago

I'm partial to using named auth keys over some kind of URL matching scheme. It makes it more explicit to end users and it basically removes the chance of there being any weird edge cases. I'm not sure about the auth key on resources, since that key is already in use in hyperquest, but something can be thought up. I think we should probably run this by Monika & Z and see what they think so we can finish this branch up real quick.

monika commented 10 years ago

Sorry sorry, I haven't had time to read through this all yet - I've been in Productionville. Do you need an answer today? @krackydile and I spoke with @Fauntleroy last night and the concept makes sense to us, but I haven't had time to review the entire conversation here, yet.

monika commented 10 years ago

I'm not into the idea of having a separate "keychain" file where we maintain all of the authentication keys, etc. I think it breaks an already shaky mental model of how resources work, and once we add in dynamic segments and pagination, it's going to get even more unmanageable (mentally).

I think removing the resources at the view level is going to take another mental leap to understand dynamic segments and the pages they're used on.

Of the options in this pull request, I think this is the easiest to comprehend.

{
    "http://proxy.storyteller.io/Fauntleroy": {
        "resources": {
            "profile": "/soundcloud/users/{username}.json",
            "favorites": "/soundcloud/users/{username}/favorites.json"
        },
        "headers": {
            "Api-Key": "e801cf0a-ee44-42d3-a29f-aff82fd0521b"
        }  
    },
    "http://services.sparkart.net/api/v1/consumer": {
        "resources": {
            "events": "/events?tags={username}"
        },
        "qs": {
            "key": "551fca3b-a134-47a3-a34a-fea6ac38af12"
        }
    }    
}

I understand that for us, most if, not all, of our resources are going to come through Storyteller.io. I think this would be easily extensible to other users who might not be using Storyteller.io.

pushred commented 10 years ago

Ideally we could support both methods, to allow for overrides and as a preference for developers like Monika who want to cut down on the framework automagic that can lead to confusion. Personally I think credentials all over the place is duplication and begs for a configuration file, but I'm probably an "advanced" user. If the credentials ever change you're also going to have to search/replace all over to make the updates. #75 at least would address the worst of that, and keep sensitive credentials committed to dozens of files out of version control.

I think removing the resources at the view level is going to take another mental leap to understand dynamic segments and the pages they're used on.

Yeah I was pretty confused about this initially too actually, I kept asking "so how are dynamic segments gonna work?" without realizing that they worked all along. I had some weird inclination towards wanting to pass their values as arguments or something when adding resources to a page. But of course the resource URLs are just considered in the context of the page/route they are added to. Still, weird nonetheless.

The original push for this was #43 and #48 which are both concerned with requesting resources for use clientside. I agree with this sentiment from #48:

Personally I'd prefer to just be able to request resources directly from Hipster with some sort of JSONP API. This is ideal because it allows us to use Hipster resources not only in Solidus sites, but also in other projects as well. I think using Hipster as a proxy for APIs that don't have JSONP support, or have unfortunate API limits, will be a big selling point for the product.

I still think there is value in being able to leverage the serverside preprocessors here. But we can experiment with going clientside all the way and come back to that issue if we find that it is actually an issue/feature needed. I suggest using the Universe API to begin with as we already support CORS there, and as I've mentioned I think we should explore a better alternative to the Universe widgets than https://github.com/sparkartgroupinc/sparkart.js leveraging some of the new tools at our disposal, such as the helpers and the potential sharing of preprocessor modules server/clientside using Browserify.

krackydile commented 10 years ago

I don't find this format confusing. I think grouping the options by the different resource endpoint URLs makes sense to me.

monika commented 10 years ago

Would centralized resources allow us to improve the SEO of pages by allowing us to have dynamic <title> elements and <meta name="description"> elements in the <head> of the page?

pushred commented 10 years ago

Not specifically, I alluded to another idea to solve that in the format example linked above though: the ability to specify a key within a resource for the value of a key (#46) in the context. I'll open a separate issue about that later today.

Fauntleroy commented 10 years ago

If we do something like this format we'll still have resource URLs coded in every view. While that's fine, I don't really see any reason to change the way resources are defined in views (since we already have resource URLs in every view). Resources are rarely changed, so I feel like the convenience of not having to retype http://proxy.storyteller.io/Fauntleroy (or more likely, copy) isn't worth changing the 1:1 relationship of resources key definition. The other changes to the view configuration in that example should really be in another ticket (like eric mentioned).

What we could do is just keep the current resource definitions in views, and then specify authentication information in auth.json. You still just have the URL strings, but you can define repetitive auth information in a single file.

pushred commented 10 years ago

It's less about retyping and more about focusing attention on the endpoint. Same reason most API consoles and documentation hide the base URLs. You could consider that a separate feature for the future though (as an optional wrapper / relative resource URL definition) and retain the traditional 1:1 format here.

Does auth.json look like this?

[{
    "http://proxy.storyteller.io/*": {
        "headers": {
            "Api-Key": "e801cf0a-ee44-42d3-a29f-aff82fd0521b"
        }    
    }
}, {
    "http://services.sparkart.net/*": {
        "qs": {
            "key": "551fca3b-a134-47a3-a34a-fea6ac38af12"
        }
    }
}]

I'd vote for expanding qs to querystring and translating to Hyperquest behind the scenes. I don't see the value of brevity here, it just introduces unnecessary ambiguity. Plus something better than Hyperquest could come along later anyway.

Anyway seems like we should just close this as we're really going back to #73 and moving the options object into auth.json to dodge changes to the object's structure.

Fauntleroy commented 10 years ago

auth.json looks like this:

{
    "http://proxy.storyteller.io/*": {
        "headers": {
            "Api-Key": "e801cf0a-ee44-42d3-a29f-aff82fd0521b"
        }    
    },
    "http://services.sparkart.net/*": {
        "query": {
            "key": "551fca3b-a134-47a3-a34a-fea6ac38af12"
        }
    }
}