Skip to content
This repository was archived by the owner on Oct 11, 2023. It is now read-only.

Resource discovery rt #9

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Resource discovery rt #9

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
367b6e5
include constants module for env[] access
mcr Nov 14, 2017
8856a90
ignore /cable routes as well
mcr Nov 15, 2017
33de4c1
include defaults[:rt] and defaults[:short] into coap list
mcr Nov 15, 2017
826ee8e
filter david routes for coap: true
mcr Nov 15, 2017
b6930a5
return resource type based upon list of resource_links
mcr Nov 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 43 additions & 6 deletions lib/david/resource_discovery.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
require 'david/server/constants'

module David
class ResourceDiscovery
include Celluloid
include David::Server::Constants

def initialize(app)
@app = app
Expand All @@ -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('&')
Copy link
Owner

@nning nning Nov 17, 2017

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?

Copy link
Collaborator Author

@mcr mcr Nov 17, 2017

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.

Copy link
Owner

@nning nning Nov 23, 2017

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?

Copy link
Collaborator Author

@mcr mcr Feb 7, 2018

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.

body_links = []

if queries.length > 0
queries.each {|q|
# XXX do these need to %-unescaped now?
(item, value) = q.split('=')

case item
when 'href'
# TODO If query end in '*', match on prefix.
# Otherwise match on whole string.
# https://tools.ietf.org/html/rfc6690#section-4.1
filtered = routes_hash.select { |link| filter(link) }
body_links = filtered.keys

when 'rt'
byebug
unless resource_links[value].blank?
body_links << sprintf("<%s>", ERB::Util.html_escape(resource_links[value]))
end

else
false
end
}
end

# TODO On multicast, do not respond if result set empty.
body = body_links.map(&:to_s).join(',')

# TODO On multicast, do not respond if result set empty.
[
200,
{
Expand Down Expand Up @@ -54,6 +83,10 @@ def clean_routes
.each { |r| delete_format!(r) }
end

def resource_links
@resource_links ||= Hash[routes.collect { |r| [r[3], r[4]] }]
end

def delete_format!(route)
route[0].gsub!(/\(\.:format\)\z/, '')
end
Expand All @@ -70,15 +103,19 @@ def filter(link)
end

def include_route?(route)
!(route[0] =~ /\A\/(assets|rails)/)
!(route[0] =~ /\A\/(assets|rails|cable)/)
end

def routes
Rails.application.routes.routes.map do |route|
Rails.application.routes.routes.select { |route|
route.defaults[:coap]
Copy link
Owner

@nning nning Nov 17, 2017

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.

Copy link
Collaborator Author

@mcr mcr Nov 17, 2017

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.

Copy link
Owner

@nning nning Nov 23, 2017

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.

Copy link
Collaborator Author

@mcr mcr Feb 7, 2018

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)

}.map do |route|
[
route.path.spec.to_s,
route.defaults[:controller],
route.defaults[:action]
route.defaults[:action],
route.defaults[:rt],
route.defaults[:short]
]
end
end
Expand Down