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

more powerful ActiveFedora::FixityService #1239

Merged
merged 1 commit into from May 4, 2017

Conversation

Projects
None yet
3 participants
@jrochkind
Contributor

jrochkind commented May 3, 2017

The fedora fixity service also reports the expected message
digest and file size. It makes sense to allow these to be accessed -- might be wanted for reporting/logging, especially on failure.

Actually cache the http response. Comment erroneously implied
it was cached before, but it wasn't. #check still forces a new
check, as it did before this commit, but verified? is a more
clear method name for return value, and will not bust response
cache.

Cleaned up some erroneous comments.

@jcoyne

This looks good, I just have a few questions regarding casting the responses.

# integer, as reported by fedora. bytes maybe?
def expected_size
fixity_graph.query(predicate: ::RDF::Vocab::PREMIS.hasSize).first.try(:object).try(:to_s).try(:to_i)

This comment has been minimized.

@jcoyne

jcoyne May 4, 2017

Member

Why are you doing to_s.to_i? Isn't the result already an integer?

@jcoyne

jcoyne May 4, 2017

Member

Why are you doing to_s.to_i? Isn't the result already an integer?

This comment has been minimized.

@jrochkind

jrochkind May 4, 2017

Contributor

Nope, it's an RDF::Literal representing a string.

@jrochkind

jrochkind May 4, 2017

Contributor

Nope, it's an RDF::Literal representing a string.

This comment has been minimized.

@jrochkind

jrochkind May 4, 2017

Contributor

I don't know if there's some way to tell the RDF::Graph what it's supposed to be so it comes back as something other than something like what you'd get from RDF::Literal.new("12121212")... but this seems good enough maybe?

@jrochkind

jrochkind May 4, 2017

Contributor

I don't know if there's some way to tell the RDF::Graph what it's supposed to be so it comes back as something other than something like what you'd get from RDF::Literal.new("12121212")... but this seems good enough maybe?

This comment has been minimized.

@jcoyne

jcoyne May 4, 2017

Member

Perfect. Thanks for the explanation.

@jcoyne

jcoyne May 4, 2017

Member

Perfect. Thanks for the explanation.

This comment has been minimized.

@awead

awead May 4, 2017

Contributor

does fixity_graph.query(predicate: ::RDF::Vocab::PREMIS.hasSize).map(&:object).map(&:to_base) work here? query is returning a set of RDF::Statement which you can call object. That in turn should be a set of RDF::Term objects, and to_base is supposed to return the base representation of the form.

@awead

awead May 4, 2017

Contributor

does fixity_graph.query(predicate: ::RDF::Vocab::PREMIS.hasSize).map(&:object).map(&:to_base) work here? query is returning a set of RDF::Statement which you can call object. That in turn should be a set of RDF::Term objects, and to_base is supposed to return the base representation of the form.

This comment has been minimized.

@jrochkind

jrochkind May 4, 2017

Contributor

@awead No, it doesn't seem to.

fixity_graph.query(predicate: ::RDF::Vocab::PREMIS.hasSize).map(&:object).map(&:to_base)
# => ["\"103945\"^^<http://www.w3.org/2001/XMLSchema#long>"]

The String "\"103945\"^^<http://www.w3.org/2001/XMLSchema#long>" is of course not very useful. I just want 103945 as an integer.

Do you think what's there now which works is acceptable?

@jrochkind

jrochkind May 4, 2017

Contributor

@awead No, it doesn't seem to.

fixity_graph.query(predicate: ::RDF::Vocab::PREMIS.hasSize).map(&:object).map(&:to_base)
# => ["\"103945\"^^<http://www.w3.org/2001/XMLSchema#long>"]

The String "\"103945\"^^<http://www.w3.org/2001/XMLSchema#long>" is of course not very useful. I just want 103945 as an integer.

Do you think what's there now which works is acceptable?

This comment has been minimized.

@awead

awead May 4, 2017

Contributor

alas, I had hoped to_base might circumvent the first..try..try business. Yeah, it's fine.

@awead

awead May 4, 2017

Contributor

alas, I had hoped to_base might circumvent the first..try..try business. Yeah, it's fine.

# the currently calculated checksum, as a string URI, like
# "urn:sha1:09a848b79f86f3a4f3f301b8baafde455d6f8e0e"
def expected_message_digest
fixity_graph.query(predicate: ::RDF::Vocab::PREMIS.hasMessageDigest).first.try(:object).try(:to_s)

This comment has been minimized.

@jcoyne

jcoyne May 4, 2017

Member

Why is to_s necessary? Isn't the object already a string?

@jcoyne

jcoyne May 4, 2017

Member

Why is to_s necessary? Isn't the object already a string?

This comment has been minimized.

@jrochkind

jrochkind May 4, 2017

Contributor

Nope, an RDF::Literal.

@jrochkind

jrochkind May 4, 2017

Contributor

Nope, an RDF::Literal.

This comment has been minimized.

@awead

awead May 4, 2017

Contributor

see my other comment about mapping object and to_base

@awead

awead May 4, 2017

Contributor

see my other comment about mapping object and to_base

This comment has been minimized.

@jrochkind

jrochkind May 4, 2017

Contributor

#to_base here gives me "<urn:sha1:1a89571e25dd372563a10740a883e93f8af2d146>". I don't really want those angle brackets. I guess I could do that and strip the angle brackets off... but what's here now seems simpler to me. What do you think?

I don't really understand the right ways to interact with the RDF::Graph stuff so appreciate your advice. But I'd also rather not fight with it if what I have here now works fine, unless there are significant problems I don't know about with what I have now?

@jrochkind

jrochkind May 4, 2017

Contributor

#to_base here gives me "<urn:sha1:1a89571e25dd372563a10740a883e93f8af2d146>". I don't really want those angle brackets. I guess I could do that and strip the angle brackets off... but what's here now seems simpler to me. What do you think?

I don't really understand the right ways to interact with the RDF::Graph stuff so appreciate your advice. But I'd also rather not fight with it if what I have here now works fine, unless there are significant problems I don't know about with what I have now?

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind May 4, 2017

Contributor

Oh, forgot to mention I also replaced some of the mocked fedora responses in the test with actual responses from my fcrepo 4.7.0, which were quite different than what was mocked before (a different RDF serialization format?), but didn't break any of the existing tests, hooray.

Contributor

jrochkind commented May 4, 2017

Oh, forgot to mention I also replaced some of the mocked fedora responses in the test with actual responses from my fcrepo 4.7.0, which were quite different than what was mocked before (a different RDF serialization format?), but didn't break any of the existing tests, hooray.

@jcoyne

jcoyne approved these changes May 4, 2017

premis:hasSize "1878582"^^<http://www.w3.org/2001/XMLSchema#long> .
EOF
}

This comment has been minimized.

@awead

awead May 4, 2017

Contributor

optional, but exceedingly awesome, would be a fixture file with that gets read in here instead of the EOFs

@awead

awead May 4, 2017

Contributor

optional, but exceedingly awesome, would be a fixture file with that gets read in here instead of the EOFs

This comment has been minimized.

@jrochkind

jrochkind May 4, 2017

Contributor

Are you thinking of a fixture file that just has the text in it, that just gets read in with File.read? Personally, I like having the data right here in the file so a developer can look at it and see exactly what is being tested. I think especially important for a mock of an external serivce I think, where it can easily get out of sync with what the service is actually returning (as I think it had), and it's good for it to be front and center for any developer looking at it.

But it's easy enough to make a fixture file if the consensus is a different opinion?

@jrochkind

jrochkind May 4, 2017

Contributor

Are you thinking of a fixture file that just has the text in it, that just gets read in with File.read? Personally, I like having the data right here in the file so a developer can look at it and see exactly what is being tested. I think especially important for a mock of an external serivce I think, where it can easily get out of sync with what the service is actually returning (as I think it had), and it's good for it to be front and center for any developer looking at it.

But it's easy enough to make a fixture file if the consensus is a different opinion?

more powerful ActiveFedora::FixityService
The fedora fixity service also reports the expected message
digest and file size. It makes sense to allow these to be accessed.

Actually cache the http response. Comment erroneously implied
it was cached before, but it wasn't. #check still forces a new
check, as it did before this commit, but verified? is a more
clear method name for return value, and will not bust response
cache.

Cleaned up some erroneous comments.
@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind May 4, 2017

Contributor

If any new commits are made before I merge, Github makes me merge/rebase master into this branch again before I can click 'merge branch'. And then wait for travis to run again. and then hope nobody else has committed to master, or else I have to do the dance again.

Is this a problem for anyone else? If people are making frequent commits, it might become impossible to ever merge, as people make commits in the time it takes travis to run.

So tell me now @awead if you'd rather me hold off on this, otherwise I'm merging as soon as I notice travis being done again. :)

Contributor

jrochkind commented May 4, 2017

If any new commits are made before I merge, Github makes me merge/rebase master into this branch again before I can click 'merge branch'. And then wait for travis to run again. and then hope nobody else has committed to master, or else I have to do the dance again.

Is this a problem for anyone else? If people are making frequent commits, it might become impossible to ever merge, as people make commits in the time it takes travis to run.

So tell me now @awead if you'd rather me hold off on this, otherwise I'm merging as soon as I notice travis being done again. :)

@awead awead merged commit 2a9def2 into samvera:master May 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jrochkind jrochkind deleted the jrochkind:more_powerful_fixity_service branch May 4, 2017

@jrochkind jrochkind referenced this pull request May 24, 2017

Merged

Fixity Overhaul #984

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment