-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add direct containers #788
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
module ActiveFedora::Associations::Builder | ||
class DirectlyContains < CollectionAssociation #:nodoc: | ||
self.macro = :directly_contains | ||
self.valid_options += [:has_member_relation, :is_member_of_relation] | ||
self.valid_options -= [:predicate] | ||
|
||
def build | ||
reflection = super | ||
configure_dependency | ||
reflection | ||
end | ||
|
||
def validate_options | ||
super | ||
if !options[:has_member_relation] && !options[:is_member_of_relation] | ||
raise ArgumentError, "You must specify a predicate for #{name}" | ||
elsif !options[:has_member_relation].kind_of?(RDF::URI) && !options[:is_member_of_relation].kind_of?(RDF::URI) | ||
raise ArgumentError, "Predicate must be a kind of RDF::URI" | ||
end | ||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
module ActiveFedora | ||
module Associations | ||
class ContainerProxy < CollectionProxy | ||
def initialize(association) | ||
@association = association | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
module ActiveFedora | ||
module Associations | ||
class DirectlyContainsAssociation < CollectionAssociation #:nodoc: | ||
|
||
def insert_record(record, force = true, validate = true) | ||
container.save! | ||
if force | ||
record.save! | ||
else | ||
record.save(validate: validate) | ||
end | ||
end | ||
|
||
def reader | ||
@records ||= ContainerProxy.new(self) | ||
end | ||
|
||
def find_target | ||
query_node = if container_predicate = options[:has_member_relation] | ||
owner | ||
else | ||
container_predicate = ::RDF::Vocab::LDP.contains | ||
container | ||
end | ||
|
||
uris = query_node.resource.query(predicate: container_predicate).map { |r| r.object.to_s } | ||
|
||
uris.map { |object_uri| klass.find(klass.uri_to_id(object_uri)) } | ||
end | ||
|
||
def container | ||
@container ||= begin | ||
DirectContainer.find_or_initialize(ActiveFedora::Base.uri_to_id(uri)).tap do |container| | ||
container.parent = @owner | ||
container.has_member_relation = Array(options[:has_member_relation]) | ||
container.is_member_of_relation = Array(options[:is_member_of_relation]) | ||
end | ||
end | ||
end | ||
|
||
protected | ||
|
||
def count_records | ||
load_target.size | ||
end | ||
|
||
def initialize_attributes(record) #:nodoc: | ||
record.uri = ActiveFedora::Base.id_to_uri(container.mint_id) | ||
set_inverse_instance(record) | ||
end | ||
|
||
private | ||
|
||
def uri | ||
raise "Can't get uri. Owner isn't saved" if @owner.new_record? | ||
"#{@owner.uri}/#{@reflection.name}" | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
module ActiveFedora | ||
# This is the base class for ldp containers, it is not an ldp:BasicContainer | ||
class Container < ActiveFedora::Base | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a comment that makes it clear that this isn't an object representation of a BasicContainer |
||
|
||
property :membership_resource, predicate: ::RDF::Vocab::LDP.membershipResource | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validations for these necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's already validated. If you remove this line the tests fail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because of LDP? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, no, because it's only using Container through that relation. I could do Container.new.save! and it'd run fine There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant a presence validation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand how that would happen. We're always building it correctly, so why have a check that costs cycles and brings no benefit? |
||
property :has_member_relation, predicate: ::RDF::Vocab::LDP.hasMemberRelation | ||
property :is_member_of_relation, predicate: ::RDF::Vocab::LDP.isMemberOfRelation | ||
|
||
def parent | ||
@parent || raise("Parent hasn't been set on #{self.class}") | ||
end | ||
|
||
def parent=(parent) | ||
@parent = parent | ||
self.membership_resource = [::RDF::URI(parent.uri)] | ||
end | ||
|
||
def mint_id | ||
"#{id}/#{SecureRandom.uuid}" | ||
end | ||
|
||
def self.find_or_initialize(id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be on AF::Base? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see a reason to move it there unless there's anyone else who would want to use this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I'm not sure what's special about Containers that would make us want to find_and_initialize here but not there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a very specific method and is hardly ever useful. We've received complaints about the API being overly broad, so I don't want to expand it unless you can show me a case where you would use it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for keeping the API narrow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also 👍 on keeping the API small, but that seems like a motivation to make this method private, or to consider a builder pattern, not to tuck it away in the subclass. My concern would be that the outcome here is an API that is both big and inconsistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @no-reply I don't know what that means. Care to make a pull request or show me what you're talking about? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @no-reply Specifically in relation to the original comment which said to move this method to AF::Base. Seems like keeping it just on containers keeps the AF::Base api smaller. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jcoyne I mean, this keeps the AF::Base API smaller, but just trades that concern with an inconsistent access API, no? I'm not willing to hold this up just for this, but it was a concern There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is not a blocker. It's a philosophical/design difference that we should probably resolve elsewhere. |
||
find(id) | ||
rescue ActiveFedora::ObjectNotFoundError | ||
new(id) | ||
end | ||
end | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
module ActiveFedora | ||
class DirectContainer < Container | ||
type ::RDF::Vocab::LDP.DirectContainer | ||
|
||
|
||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
module ActiveFedora | ||
class FileRelation < Relation | ||
def load_from_fedora(id, _) | ||
klass.new(klass.id_to_uri(id)) | ||
end | ||
end | ||
end |
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.
{name} ?
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.
?
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.
where is name defined?
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.
In the superclass
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.
Uh, nope. Good catch.
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.
Oh, yes, it is in the superclass: https://github.com/projecthydra/active_fedora/blob/master/lib/active_fedora/associations/builder/association.rb#L9