Skip to content
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

Fixity Overhaul #984

Merged
merged 26 commits into from May 19, 2017
Merged

Fixity Overhaul #984

merged 26 commits into from May 19, 2017

Conversation

@jrochkind
Copy link
Contributor

jrochkind commented May 16, 2017

The fixity-checking code was in kind of bad shape

On further analysis, I think much of it dated from both before FC4, and before part of it had been made an async ActiveJob. And while things were changed enough for tests to pass at that point, there was broken func ("fixity status" on FileSet show page passing tests cause they were mocked but not actually working); things not quite sensibly for a multi-file multi-version environment; API that didn't quite make sense for async; method params, db attributes and local vars that no longer accurately described what they were; and some crufty implementation.

I tried to fix it all up, the interrelatedness of the classes means this is kind of a big PR, but I tried to do individual test-passing commits (not always succesfully).

After this PR:

  • "Fixity status" on FileSet show page actually shows fixity status. Taking account of the fact that there may be more than one binarystream involved (multiple versions and/or files), it's more than just a simple 'pass' or 'fail', especially in the (very very rare) case of 'fail', but I tried to fit it into the 180px column it was in (didn't want to mess with that page which others are already discussing. Closes #188

    • Actual human readable message creation extracted to a new FixityStatusService
  • The ChecksumAuditLog records being created will include expected_result if using an ActiveFedora that returns it (respond_to? check).

  • The pruning of old ChecksumAuditLog records could have been deleting too many, it's now multi-bytestream-per-fileset aware.

  • The after_fixity_check_failure hook was not actually passing enough info to know what failed, in a multi-bytestream-per-fileset world. It now passes the ChecksumAuditLog record itself, with everything that's been recorded. (The actual action/message of the default FixityCheckFailureService is unchanged, but at least the right info is being passed for a different one)

  • FileSetFixityCheckService has some options on init to: run checks in foreground instead of async (only way to get immediate results); check latest version(s) only instead of all versions; override max_days_between_fixity_checks from global configuration.

  • Lots of arg name changes -- it was confusing what the args represented, in addition to renaming, made some of them keyword args so you have to pay attention to what it is you're passing. Lots of general code cleanup and modernization.

    • The ChecksumAuditLog AR record has renamed/typed columns to accurately convey what they actually are -- didn't change what they were from previous code, just made it work and be named educationally. Including a boolean for "passed" rather than an integer that previously could also be 999 meaning, I guess, "in progress" -- that was not really neccessary.
    • Lots more class-level and method-level docs explaining what things are meant to do.

This code is definitely not perfect, there is lots of room for improvement. But after 2+ weeks figuring out what it did, what it was trying to do, what it should be trying to do, and making it so -- I think it's good enough, working, and a good basis for future work someday.

One thing I don't like is that the recorded checked_uri in ChecksumAuditLog (usually a version URI, but sometimes a file URI if the fcrepo does not have versions enabled, or the caller chooses to use it this way -- these semantics were inherited) is a uri, instead of a fedora id like the file_set_id and file_id. This could make it sensitive to hostname changes on the fedora server. But there just didn't seem to be any way to get an id out of what I had without other changes in other parts of the stack I didn't have the stomach for.

UI things I'd like but haven't gotten to yet (and may not):

  • I think the 'versions' tab on File/edit screen, which is the only place that lists "versions", probably ought to put a fixity status next to each version, since fixity checks are actually version-specific.

  • I think there ought to be an admin dashboard fixity status report, either on dashboard itself or a separate page or a combo of both, that lists any current failing fixity checks, as well as general fixity status (range of dates latest fixity checks were made on, number of fixity checks, whatever).

I'm hoping this is useful to Hyrax; I have limited time to make any major changes, but am happy to discuss the code. I will probably be backporting this to our local (sufia 7.3) app regardless of it's disposition here.

jrochkind added 21 commits May 4, 2017
This matches what it actually is in the code currently.

The FixityCheckJob says "This URI could include the actual resource (e.g. content) and the version to fixity check... but it could also just be [a file URL]"

It isn't neccesarily a version. It IS always created as a URI -- even when
it's a version, it's the version uri, not the version label. What it actually
is, is the URI that was passed to ActiveFedora::FixityCheckService to check,
which is fine.

Accurate names make code less confusing, trying to make it clearer
what this code does.
With some docs of what's going on too
from ChecksumAuditLogs on record.
May be for _multiple_ files/versions attached to FileSet
instead of it's existing implementation that was not working right.
* You can get the boolean from ChecksumAuditLog.
* FileSetFixityCheckService needs the whole ChecksumAuditLog.
* Docs more clear on you only get this if calling non-async!
And clarify about what you get as a return value when running async_jobs: you get nothing
which I _think_ is what it actually returned before, at any rate
FixityChecksController seemed to expect it. So much mocking
may have made some false test passing.
insisting on async_jobs false so it can actually get a response,
naming keys accurately for what they actually are.
In a multi-file/multi-version environment, it was not getting enough information to actually identify what failed. Just pass it the ChecksumAuditLog, which by definition must have all the info needed. No need for primitive obsession.

Also move the user/depositor lookup into the Failure service itself. I suspect sending to depositor
is not desired behavior; but it's default/legacy for now; at least now local apps have
enough info to easily customize and change notification
and updated for changes.
with args that enforce meaning to avoid confusion.
With better semantics for prune_history
So it'll fit in the 180px (!) column it's in. No desire to change that front-end UI right now.
…st only with future AF version that has it
@jrochkind

This comment has been minimized.

Copy link
Contributor Author

jrochkind commented May 16, 2017

Hey @aeschylus et al, not sure what the 'in_progress' tag means, but I consider this ready for review/merge, and don't necessarily plan on doing more work on it.

The things I mentioned as possible future improvements are, but I think this is acceptable for merge now and an improvement over what's there.

Subject to consultation with reviewers of course, but i'm ready for that!

@jcoyne

This comment has been minimized.

Copy link
Member

jcoyne commented May 17, 2017

@jrochkind the in_progress tag is added by waffle.io and you can disregard it.

Copy link
Member

jcoyne left a comment

I am happy with the documentation added and the clarifications that have been made here. I'm not hung up on the deprecations as I doubt that anyone is really overriding/calling these methods, but there could be some concern here so I'd like another reviewer to also take a look at this.

def create
render json: fixity_check_service.fixity_check
end

protected

def fixity_check_service
file_set = ::FileSet.find(params[:file_set_id])
FileSetFixityCheckService.new(file_set)
@fixity_check_service ||=

This comment has been minimized.

Copy link
@jcoyne

jcoyne May 17, 2017

Member

Is this ever called more than once per request? do we need to memoize? Can you add documentation about why you are setting async_jobs: false

This comment has been minimized.

Copy link
@jrochkind

jrochkind May 17, 2017

Author Contributor

I am in the habit of memoizing everything in a controller, so if it's later called more than once in a view, you just don't need to think about it. Does it hurt, or do you think this is a bad pattern in some way?

This comment has been minimized.

Copy link
@jrochkind

jrochkind May 17, 2017

Author Contributor

I can add docs about async:false. We can also discuss whether the design is wise.

Basically, because async:false is the only way to be assured to get a response that you can return immediately. The alternative would be more complicated code that somehow returned "in progress" for bytestreams without a fresh fixity check record.

I went with the simple approach in part because I don't believe this action is actually used by anyone. It's not used by existing hyrax code so far as I can tell. I think it's a leftover from a previous (pre FC4 even?) version of sufia that used it for "fixity status" display, but it hasn't for a while.

I wanted to leave the action here because it actually does seem hypothetically useful to have an HTTP API on fixity status for external integration. But I didn't want to spend too much time on it making it perfect, when just guessing on use cases.

Another alternative would be removing this action/controller altogether, which I'd also be open to.

This comment has been minimized.

Copy link
@jcoyne

jcoyne May 17, 2017

Member

I don't really care about the memoization, it just seemed unnecessary to me. I wanted to be sure I wasn't missing anything. I would like you to add a comment though.

# @param uri [String] uri - of the specific file/version to fixity check
# @param file_set_id [FileSet] the id for FileSet parent object of URI being checked.
# @param file_id [String] File#id, used for logging/reporting.
def perform(uri, file_set_id:, file_id:)

This comment has been minimized.

Copy link
@jcoyne

jcoyne May 17, 2017

Member

Changing the erasure of a method is a breaking change, so this should be for Hyrax, 2, right? Do you think we need to deprecate the old interface?

@@ -1,20 +1,49 @@
class ChecksumAuditLog < ActiveRecord::Base
# TODO: this method doesn't seem to be used. Remove?
def self.fixity_check_log(id, path, version_uri)

This comment has been minimized.

Copy link
@jcoyne

jcoyne May 17, 2017

Member

should this method be deprecated before removal?

This comment has been minimized.

Copy link
@jrochkind

jrochkind May 17, 2017

Author Contributor

In general re deprecations: This stuff had to change so much that adding deprecations everywhere would be a major increase in complexity. I think that the former methods probably were not used by anyone, because, well, they didn't really work. So I guess I'm making the argument that the previous signatures and methods were effectively bugs. I am normally pretty pro-backwards-compat, but in this case, I don't think backwards compat with probably-unused broken architecture is a priority.

I recognize other opinions may be reasonable though! But I don't know if I personally have time to go back and make sure all existing (public?) methods and signatures still work with deprecation.

# check.
# @param latest_version_only [Booelan]. Check only latest version instead of all
# versions. Default false.
def initialize(file_set,

This comment has been minimized.

Copy link
@jcoyne

jcoyne May 17, 2017

Member

Changing the erasure of a public method. Do we need a deprecation?

This comment has been minimized.

Copy link
@jrochkind

jrochkind May 17, 2017

Author Contributor

I assume you mean 'signature', not 'erasure'? See above on my current attitude toward deprecation. The previous signature was just too easy to be mis-used (and was being mis-used in existing code), it needed a new signature that was clearer on semantics and intent.

I guess one could leave the signature the same (subject to misuse) for sake of backwards compat, but I think this would be unfortunate. Or one could add deprecation with previous signature working here and in other places, with significant increase in complexity of code.

Originally I wanted to maintain backwards compat everywhere, but the more I got into it, the more I arrived at thinking this stuff had to be more drastically altered to have a transparent and clear API.

This comment has been minimized.

Copy link
@jcoyne

jcoyne May 17, 2017

Member

Yes, I'm using the wrong term. Signature is a better choice.

# This may trigger fixity checks if required
# @param [Hydra::PCDM::File] file the file to get the fixity check status for,
# defaults to the original_file.
def human_readable_fixity_check_status(file = file_set.original_file)

This comment has been minimized.

Copy link
@jcoyne

jcoyne May 17, 2017

Member

Should this method be deprecated before removal?

# Returns the set of most recent fixity check status for each version of the
# content file
# @param [Hash] log container for messages, mapping file ids to status
def fixity_check(log = {})

This comment has been minimized.

Copy link
@jcoyne

jcoyne May 17, 2017

Member

Should this method be deprecated before removal?

This comment has been minimized.

Copy link
@jrochkind

jrochkind May 17, 2017

Author Contributor

Thus method hasn't actually been removed, the git diff has just separated it from where it remains since there were so many changes to this file. It does now take no params instead of a log param of unclear utility that was not being used internally in hyrax.

attr_reader :log_date
attr_reader :log_date, :checksum_audit_log, :file_set

def initialize(file_set, checksum_audit_log:)

This comment has been minimized.

Copy link
@jcoyne

jcoyne May 17, 2017

Member

Should the old interface to this method be deprecated?

# See FileSetFixityCheckService and ChecksumAuditLog for actually performing
# checks and recording as ChecksumAuditLog objects.
class FixityStatusService
include ActionView::Helpers::TagHelper

This comment has been minimized.

Copy link
@jcoyne

jcoyne May 17, 2017

Member

This to me seems like an odd include for a service object. I would think that an object that emits HTML would be a presenter.

This comment has been minimized.

Copy link
@jrochkind

jrochkind May 17, 2017

Author Contributor

I don't really understand the hydra architecture here. I think I went with service object simply because it was before -- I separated out from one service object to another. And maybe I misunderstood a comment you made in slack that I thought was recommending extracting this to it's own service object.

Happy to change it to a 'presenter' object if you think that's better. I don't really understand the hydra 'presenter' architecture, can you share an analogous presenter object I can use as a guideline? Can this still be it's own presenter object that does nothing but status message, does that fit into the architecture? I worry that trying to shoehorn this into some existing presenter object that also does other stuff would be a bigger harm to backwards compat to people who have overridden that presenter object (as is encouraged I think?).

While most of the public API effected in this PR I think was not used by anyone, "display a fixity status message for FileSet" of course is used in every sufia/hyrax app, and I tried to keep the code path to that as backwards compat as possible (even leaving the cover method in FileSetFixityCheckService, for those who have presenters calling it).

This comment has been minimized.

Copy link
@awead

awead May 17, 2017

Contributor

I don't think of services as rendering any html. In fact, I don't even think presenters should render html... that's what views are for. However, if you need the ability to call this service to display html in different places, then I suppose this could be a good solution. Otherwise, I think a presenter/view combo would be better.

The problem with the html-service is that it's difficult to override. I can't define my own service and us it in place of this, I have to override the whole thing, and I might only need to make cosmetic changes, which suggests a view would be better. Our current pattern is to define presenters and services in the controller so one can override them and define their own there.

If you wanted to go with a presenter, then it's just a plain ruby class that encompasses a fixity status, whatever that is. If it's just a single record from the database table, then you might not need one and passing the AR object to the view is enough. If a "fixity status" is multiple records, or a single record with some logic anchored around it, then the presenter can take care of that and then the view receives the presenter object and renders its contents with the appropriate html tags, etc.

@jrochkind

This comment has been minimized.

Copy link
Contributor Author

jrochkind commented May 17, 2017

I agree this could use a third set of eyes/opinions. Any suggestions as to who might be a good interested party? @awead? @mjgiarlo? @tpendragon, you expressed some interest in this topic?

@@ -1,42 +1,63 @@
class FixityCheckJob < Hyrax::ApplicationJob

This comment has been minimized.

Copy link
@hackmastera

hackmastera May 17, 2017

Contributor

It seems like we want to keep this? came from a recent PR. #951

This comment has been minimized.

Copy link
@jrochkind

jrochkind May 17, 2017

Author Contributor

I meant to keep < Hyrax::ApplicationJob, if it's not there it came from a merge with master gone wrong. good catch.

@jcoyne

This comment has been minimized.

Copy link
Member

jcoyne commented May 17, 2017

@jrochkind I'd be 👍 if you move the fixity_status_service.rb to a presenter (or split it into two classes, one of which is a presenter).

Happy to change it to a 'presenter' object if you think that's better. I don't really understand the hydra 'presenter' architecture, can you share an analogous presenter object I can use as a guideline? Can this still be it's own presenter object that does nothing but status message, does that fit into the architecture? I worry that trying to shoehorn this into some existing presenter object that also does other stuff would be a bigger harm to backwards compat to people who have overridden that presenter object (as is encouraged I think?).

Yes, a presenter object could be very narrowly scoped and we can have presenters nested by other presenters. A presenter pattern is also called the view-model, so think of it as modeling the data and showing it to the user. Anything that is mostly about rendering strings and html is most likely a presenter.

@jrochkind

This comment has been minimized.

Copy link
Contributor Author

jrochkind commented May 17, 2017

@jcoyne do I understand correctly presenter would/could have almost the exact same API it has now, (.new(FileSet), and .file_set_fixity_status), basically just be in a different part of the filesystem with a different class name? That's no problem and makes sense to me, I can do that.

@jcoyne

This comment has been minimized.

Copy link
Member

jcoyne commented May 17, 2017

@jrochkind yes. It sounds like we understand each other.

@jrochkind jrochkind force-pushed the fixity_refactor branch from 3016ae8 to f2777f0 May 17, 2017
@jcoyne
jcoyne approved these changes May 19, 2017
def render_file_set_status
@file_set_status ||=
if relevant_log_records.empty?
"Fixity checks have not yet been run on this object"

This comment has been minimized.

Copy link
@mjgiarlo

mjgiarlo May 19, 2017

Member

There are some strings in this PR that I'd like to see handled via i18n, but this PR is big enough and has waited long enough that it's reasonable for this to be ticketed and worked on subsequently IMO.

@mjgiarlo

This comment has been minimized.

Copy link
Member

mjgiarlo commented May 19, 2017

Thank you for the contribution, @jrochkind!

@mjgiarlo mjgiarlo merged commit 7d178e1 into master May 19, 2017
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 96.757%
Details
@mjgiarlo mjgiarlo deleted the fixity_refactor branch May 19, 2017
@mjgiarlo mjgiarlo removed the in progress label May 19, 2017
@jrochkind

This comment has been minimized.

Copy link
Contributor Author

jrochkind commented May 21, 2017

Thanks @jcoyne / @mjgiarlo ! Can you clarify, will this be in Hyrax 1.0, or too late for that -- and I should expect it in next 1.x, or not til a 2.0?

I have no opinion, you guys know what's best, just will use the info to effect my planning, thanks!

(And yep, I agree it oughta be i18n ideally -- but it wasn't i18n'd before either, and I just ran out of steam, especially since the text I put in to give a more complete report on failure got somewhat complex. Thanks @mjgiarlo!)

@jcoyne

This comment has been minimized.

Copy link
Member

jcoyne commented May 21, 2017

@jrochkind presently master is targeting 2.0, if you want it in 1.0, submit a PR that cherry-picks these commits to the 1-0-stable branch.

jrochkind added a commit that referenced this pull request May 22, 2017
* with additional/fixed specs.
* fixes bugs in behavior added in #984
jrochkind added a commit that referenced this pull request May 22, 2017
* with additional/fixed specs.
* fixes bugs in behavior added in #984
@jrochkind

This comment has been minimized.

Copy link
Contributor Author

jrochkind commented May 24, 2017

@jrochkind presently master is targeting 2.0, if you want it in 1.0, submit a PR that cherry-picks these commits to the 1-0-stable branch.

And yet, somehow these commits made it into 1.0.0 anyway. Without separate commits cherry-picking. I totally have no idea how to keep track of or predict these things.

jrochkind added a commit that referenced this pull request May 24, 2017
previous migration from #984, postgres did not like.

This way could be slow, as it needs to fetch and save each object individually,
but I can't think of anything better.

Those who already generated this hyrax app into a local app may have to manually
copy the migration. If this hadn't been in a release yet, that wouldn't seem
like a problem -- but seems it made it into 1.0 somehow?

Fixes #1035

As a bonus, it's reversible now.
jcoyne added a commit that referenced this pull request May 25, 2017
* Change data from integer to boolean in db-independent way

previous migration from #984, postgres did not like.

This way could be slow, as it needs to fetch and save each object individually,
but I can't think of anything better.

Those who already generated this hyrax app into a local app may have to manually
copy the migration. If this hadn't been in a release yet, that wouldn't seem
like a problem -- but seems it made it into 1.0 somehow?

Fixes #1035

As a bonus, it's reversible now.

* Do the update using SQL

* Use correct table name
jcoyne added a commit that referenced this pull request May 26, 2017
* Change data from integer to boolean in db-independent way

previous migration from #984, postgres did not like.

This way could be slow, as it needs to fetch and save each object individually,
but I can't think of anything better.

Those who already generated this hyrax app into a local app may have to manually
copy the migration. If this hadn't been in a release yet, that wouldn't seem
like a problem -- but seems it made it into 1.0 somehow?

Fixes #1035

As a bonus, it's reversible now.

* Do the update using SQL

* Use correct table name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.