-
Notifications
You must be signed in to change notification settings - Fork 11
Resource discovery rt #9
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very mich for your PR!
end | ||
|
||
def routes | ||
Rails.application.routes.routes.map do |route| | ||
Rails.application.routes.routes.select { |route| | ||
route.defaults[:coap] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I am not convinced, it should be necessary to add routes explicitly although it could be helpful in a bigger app that focuses primarily in http.
I think, I would like it to be configurable with all routes bring included by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, so I didn't want my ActiveScaffold routes showing up in the CoAP interface... I agree that a configuration that includes all routes would be a good thing. Where should I put that configruation?
Also I was initially trying to annotate the routes using the discoverable clause in the controller, and I think that should be possible as well. I just didn't find an elegant way yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rails only configuration options are perfectly fine in lib/david/railties/config.rb
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A configuration option in lib/david/railties/config.rb would apply to all applications... I'm thinking that it belongs in the application's config/routes.rb. (Sorry for coming back to this after such a long time. I've been in OpenSSL DTLS hell. It works now though)
@@ -16,11 +19,37 @@ def _call(env) | |||
|
|||
@env = env | |||
|
|||
filtered = routes_hash.select { |link| filter(link) } | |||
body = filtered.keys.map(&:to_s).join(',') | |||
queries = @env[QUERY_STRING].split('&') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the filtering by rt
not be done in the filter
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first, I tried to do exactly that. (see: AnimaGUS-minerva@d463c9d) The problem is that one then has complex logic to decide how many times to return a resource which may be duplicated by others, so I changed my mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we dedup them after filtering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about dedup, and I tried that, but it just seemed like a backwards way to do things. Can you explain why you think it's better to do this in filter? It seems like really we are doing two different things in this query, and why conflate them?
I also am thinking that if we moved the resource_discovery out of david and somehow into the application, that it would be easier to support Sinatra.
This lets one add three attributes to the routes in config/routes.rb:
coap: true
unless this is set, then the route will not show up in /.well-known/core resource discovery.
rt: 'string'
short: '/string'
will return /string as part of a query for /.well-known/core?rt=string.
In addition, the /cable URLs are omitted by default, although perhaps that filter is unimportant given coap: being false by default.