[
https://issues.apache.org/jira/browse/SHINDIG-159?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12585026#action_12585026
]
Kevin Brown commented on SHINDIG-159:
-------------------------------------
Some general comments. Since we don't have a decent review system I'll just put
them all here. I'm sure half of these are just because of merge conflicts vs
your previous patch.
A bunch of these lines go past 80 columns. For shame.
OAuthStoreImpl is a weird name. DefaultOAuthStore maybe? (I have a TODO to
rename any Basic* class that is actually suitable for most deployments to
Default*, and reserve Basic* for things that really aren't useful for real
sites).
GadgetTokenStore doesn't appear to have anything to do with the other
"GadgetToken". This confused me. Would OAuthTokenStore be more appropriate?
You can replace this ugly bit:
for (Iterator<?> i = oauthConfig.keys(); i.hasNext();) {
with:
for (String key : JSONObject.getNames(oauthConfig))
In general I think we should move more towards a hierarchy of exceptions and
not rely on the ever-growing enumerations in GadgetException. It looks like
you've done a mix here. Any particular reason?
It's not clear to me what the oauth feature.xml is for -- it appears to just be
empty. feature.xml files are really only there for cases where there's js code
that needs to be triggered. Is this just to allow you to use <Require
feature="oauth"> without getting an error ? If so, it's probably fine until we
have oauth support fully baked into the standard. You could also technically do
an <Optional feature="oauth"> here and it would work without the dummy xml
file, which would probably be a better choice anyway to maximize portability.
I'm thinking that the oauth config should be moved to the syndicator config,
under the "oauth" key. Not really much value in having a separate file here,
and the syndicator config can be injected. I recently patched syndicator config
to let you do xpath-like querying so you can do stuff like
config.get("foo/bar/baz") on nested config data. It's pretty useful. This makes
it easier to support development and production configs simultaneously.
What's with this line? Obfuscated java contest? :)
String message = new StringBuilder()
.append("gadget spec is missing oauth feature section")
.toString();
Why is OAuthFetcherFactory taking a GadgetSpecFetcher annotated
RemoteContentFetcher? It shouldn't be fetching specs. You probably just want
the stock fetcher for this. Actually, it looks like you never even use this, so
you can probably just remove it.
It seems like the ProxyHandler is dealing with too many oauth-specific details
(and that fetcher "chain" isn't really a chain..it's just holding two objects
together from what I can see). It seems like it would be better to have the
OAuthFetcher return some headers and extract those in a generic way. I can see
that functionality being generically useful for other fetcher types as well,
like caches. We don't want to return all headers in the json response (most are
meaningless and just increase latency), but maybe we could annotate them with
something like "X-shindig-" and then we'd have a convenient way to propagate
messages from fetchers back up the chain without making ProxyHandler aware of
specific nuances of individual fetcher types.
I think "https" isn't actually supported by our current ad-hoc http client. If
we replace that with the apache commons http client in
BasicRemoteContentFetcher though, we should be good.
Uh....there's some other minor stuff, but code reviews of patch files sucks.
Where's that damned review board setup when I need it?
> OAuth support in Shindig
> ------------------------
>
> Key: SHINDIG-159
> URL: https://issues.apache.org/jira/browse/SHINDIG-159
> Project: Shindig
> Issue Type: Bug
> Components: Gadgets Server - Java
> Reporter: Brian Eaton
> Attachments: full-oauth.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
