samvera / noid-rails Goto Github PK

This follows the proposed maintenance reorganization within samvera/maintenance#137

Descriptive summary

This repository’s default branch has already been renamed using GitHub’s renaming tool. Links that reference the old branch name are automatically forwarded to the new default branch. But string references are not automatically updated.

Check this repository for hard-coded string references to the legacy “master” default branch and update them to the new default branch name “main.”

Important places to check include, but are not limited to:

• READMEs
• wikis
• other documentation

NOTE: READMEs, wikis, and other documentation are important to update to avoid confusion and correct errors in long lasting documentation.

Less common places to check:

• code
• Issues/PRs

NOTE: String references to themaster branch in Issues, PRs, and code are uncommon. Also Issues and PRs are temporal in nature, making it less critical to update those occurrences.

Rationale

Git's default "master" branch derives from "master/slave" jargon which perpetuates systemic racist language and systems (see email Replacing "master" reference in git branch names). To uphold our Code of Conduct, we must move away from the term "master" in our technical language (as well as words like blacklist or whitelist).

Related work

• Background on the renaming effort is available in the working group notes.
• See the GitHub Renaming documentation for the changes that GitHub made during the renaming process.

The database-backed minter (db branch) is coming along well, but there are some design questions that need broader vetting.

LOCKING: Obviously we will use a DB transaction, but the transaction only helps for our connection. The point of the DB for horizontal scalability is that potentially many connections will be concurrent. What kind of locking do we want to use?

1. optimistic -- downside: race condition, ActiveRecord::StaleObjectError
2. pessimistic -- downside: deadlock
3. db-specific (e.g. LOCK IN SHARE MODE) or other?

Pick your poison.

FAILSAFE: The point of locking is that one way or another only one connection/thread writes to the DB at a time. That means by design somebody loses (at write time for optimistic, at read time for pessimistic). The mint method is designed to loop forever until it gets a novel NOID, so presumably catching the failure there and retrying is OK, but at a certain scale, after a certain amount of failure, cycling forever in race/deadlock is counter-productive.

So, based on the lock you prefer, what tolerance for failure do we have here? Raise after 100 StaleObjectError or timeout after X to avoid deadlock?

Add the following branch renaming language to the README for this repository.

## Contributing 

If you're working on PR for this project, create a feature branch off of `main`. 

This repository follows the [Samvera Community Code of Conduct](https://samvera.atlassian.net/wiki/spaces/samvera/pages/405212316/Code+of+Conduct) and [language recommendations](https://github.com/samvera/maintenance/blob/master/templates/CONTRIBUTING.md#language).  Please ***do not*** create a branch called `master` for this repository or as part of your pull request; the branch will either need to be removed or renamed before it can be considered for inclusion in the code base and history of this repository.

Rationale

Git's default "master" branch derives from "master/slave" jargon which perpetuates systemic racist language and systems (see email Replacing "master" reference in git branch names). To uphold our Code of Conduct, we must move away from the term "master" in our technical language (as well as words like blacklist or whitelist).

Related work

• Background on the renaming effort is available in the working group notes.
• See the GitHub Renaming documentation for the changes that GitHub made during the renaming process.

Description

The identifier_in_use attribute can be changed by the identifier_in_use attr_writer, but it will not stay changed. Each time you call the reader it is changed back to the lambda that always returns false.

Expected

      def identifier_in_use
        @identifier_in_use ||= lambda do |_id|
          false
        end
      end

Actual

      def identifier_in_use
        @identifier_in_use = lambda do |_id|
          false
        end
      end

https://github.com/samvera/noid-rails/blob/master/lib/noid/rails/config.rb#L29

This is something I hadn't looked closely enough at before pushing the PR previously. If you override id generation, Fedora will properly be handed the desired path - for instance, be/19/d0/b2/2514nz446. If you do not override id generation and let Fedora do its UUID thing, Fedora chooses the path - for instance, 4a/48/49/6a/4a48496a-d56b-44a0-9836-e3381dfe13bd.

With UUIDs, the bucketing system is able to use the first 8 characters because they are never predictable. With NOIDs, there is a predictability, which is why I pushed up #3 to begin with.

The problem here can originate if you do not specify IDs prior to saving in Fedora for every single object you ever create. If there is any situation where an object is created in Fedora without both a predetermined PID and the overridden path translation code, things will be essentially unfindable by the app.

For most users this probably isn't an issue, but it's the sort of problem that could hit unexpectedly and won't make a lot of sense if it does.

I think the best long-term solution would be to let Fedora bucket by UUID and then have our pids be simply a piece of data associated with the object, rather than the pids being the definitive ID. I have no idea how to make this happen, as I (obviously) am not all that familiar with the Hydra stack, especially Fedora 4 / Hydra 9.

Barring that, it might be good to have the treeifier just return the id as-is unless it's a valid noid. Then anything not minted as a noid could just use the built-in fedora treeification. This is sort of weird and magicky, but it might be a good fix for the vast majority of use cases (e.g., creating a dummy asset in a test where the minting logic is deliberately not wanted).

Descriptive summary

This repository’s default branch has already been renamed to main using GitHub’s renaming tool. In order to preserve automatic redirection of links that reference the old branch name master to the new default main branch, a branch with the old name should not be recreated.

CircleCI can be used to prevent the recreation of the old default branch name by preventing PRs with a branch named master from being merged by causing a test failure during continuous integration.

Rationale

Git's default "master" branch derives from "master/slave" jargon which perpetuates systemic racist language and systems (see email Replacing "master" reference in git branch names). To uphold our Code of Conduct, we must move away from the term "master" in our technical language (as well as words like blacklist or whitelist).

Expected behavior

If a PR is submitted with a branch named master, the continuous integration tests should fail.

Actual behavior

If a PR is submitted with a branch named master, the continuous integration tests will not fail because of the branch name.

Related work

Background on the renaming effort is available in the working group notes.

Derived from samvera/maintenance#89

Currently:

uri = http://localhost:8983/fedora/rest/test/hh/63/vz/22/hh63vz22q/members"
p ActiveFedora::Base.uri_to_id(uri)
"members"

it should be

"hh63vz22q/members"

You do not have permission to push to this gem.

This gem should probably be added to https://github.com/projecthydra/hydra/blob/master/script/grant_revoke_gem_authority.rb and then @mjgiarlo (the only person with access) can run it.

"http://localhost:8983/fedora/rest/test/te/st/5/test5/files"
(byebug) p ActiveFedora::Base.uri_to_id(uri)
"files"

I would expect test5/files instead

getting the wrong id for short ids.

This is derived from samvera/maintenance#119, and follows the guide in https://guides.rubygems.org/mfa-requirement-opt-in/

ActiveFedora::Noid.config.translate_uri_to_id.call(RDF::URI('http://127.0.0.1:8986/rest/test/sf/26/85/66/sf2685667/list_source#g70294565559000')
=> "sf2685667/list_source"

But the default ActiveFedora one doesn't:

ActiveFedora::Core::FedoraUriTranslator.call(RDF::URI('http://127.0.0.1:8984/rest/dev/list_source#g70294565559000'))
=> "list_source#g70294565559000"

The default minter has been the file-based SynchronizedMinter. With the major version bump and availability of DB-backed minter, do we want to shift the default to that?

Documentation under the "ActiveFedora" integration is missing information about samvera-deprecated/sufia@21fe2ac#diff-f40d980889b56e5c9b6b53f6abead3feR69 .

This code is in production currently, yes?

Ruby 2.7.0 was released on 12/25/2019, and in accordance with the charter of the current phase of the Component Maintenance Working Group, the CircleCI configuration for this should be updated.

Derived from samvera/maintenance#76

The Renaming Branch Working Group is in the process of renaming the default branch from master to main in Samvera and Samvera-Labs repos. This brings repositories into compliance with the Samvera Community Code of Conduct (https://samvera.atlassian.net/wiki/spaces/samvera/pages/405212316/Code+of+Conduct) and language recommendations (https://github.com/samvera/maintenance/blob/master/templates/CONTRIBUTING.md#language).

This issue will be complete when the master has been renamed to main.

Related issues will have a title beginning with RENAME.

Pulled from my retrospective comment on #40.

I think it is problematic to have extracted locking to outside the transaction. Locking for update now happens in multiple places, including on every read. Previously only a transaction could block another transaction. Now a read can.

read and write! can now block each other: each calls instance which calls MinterState.lock.find_by, so they are not working on the same object, but each has a lock.

read and write! being public (and superficially documented) methods is sorta a problem. I mean, the Base class says read returns a Hash representing the minter state. Both it and the concrete class should be able to be more specific. (E.G., are the keys symbolic, string or mixed? what's this magic JSON marshalling?) Also, since it is public, a consumer might reasonably call read and be unaware that a locking operation is the result.

By my interpretation, read does not need SELECT ... FOR UPDATE for an instance it converts to a Hash, then throws away. Unlike before, no update ever comes for that object! The point of having the transaction was to put the read and the save! on the same object (as previously the case).

Makes production deploys fail.

Because consumer may not be using rails. (if defined? Rails or similar)

See: https://github.com/projecthydra-labs/active_fedora-noid/blob/master/lib/active_fedora/noid/service.rb#L22-L24

      def default_minter
        @minter ||= ActiveFedora::Noid::SynchronizedMinter.new
      end

This means the instance variable is set twice during initialization and that the default_minter method has an unnecessary side-effect.

Rails version 6.0.0 was released on 08/16/2019, and in accordance with the charter of the current phase of the Component Maintenance Working Group, the CircleCI configuration for this should be updated.

Derived from samvera/maintenance#17

Migrating an object with a 7 digit id was yielding empty path segments in the members URI when CC was configured with the default (noid) id-uri translators. Reverting to AF basic fixed the problem.

Example: 1667751 -> /dev/16/67/75/1//1667751/members

Running rails generate active_fedora:noid:install results in the following error:

[WARNING] Could not load generator "generators/active_fedora/noid/install_generator". Error: undefined method `expand_path' for ActiveFedora::File:Class.
/Users/dixonj16/.rvm/gems/ruby-2.3.0/gems/active_fedora-noid-2.0.0.beta1/lib/generators/active_fedora/noid/install_generator.rb:4:in `<class:InstallGenerator>'
/Users/dixonj16/.rvm/gems/ruby-2.3.0/gems/active_fedora-noid-2.0.0.beta1/lib/generators/active_fedora/noid/install_generator.rb:3:in `<module:Noid>'

This can be reproduced by creating a new rails app and adding the following gems.

gem 'active_fedora-noid', '2.0.0.beta1'
gem 'active-fedora', '10.1.0'

Derived from samvera/maintenance#77

class LocalNames
  include Blacklight::SearchHelper
  include Blacklight::Configurable

  copy_blacklight_config_from CatalogController

  blacklight_config do |conf|
    # conf.search_builder_class = LocalNameSearchBuilder
    config.default_solr_params = {
      'qf' => 'foaf_name_tesim',
    }
  end

  def initialize
    puts blacklight_config.default_solr_params
  end
end
LocalNames.new

This prints out the CatalogController defaults, not the defaults I set in this class. Is that right?

Derived from samvera/maintenance#16

https://github.com/projecthydra/active_fedora-noid/blob/master/lib/active_fedora/noid/minter/db.rb#L9

The call to instance locks the object, yet we never update or free it.

The maintenance template for CONTRIBUTING.md is used by all Samvera repos. Replace the current version with the updated maintenance template version.

See discussion in #5.

Reliance on filesystem for statefile prevents horizontal scaling of the application (as we need in HyBox). Reliance on single path in filesystem also blocks multitenancy. The solution for either of these is to use a db-backed "statefile".

Design questions

Should this be a separate class here akin to SynchronizedMinter? a fork of this project? a totally independent gem?

For performance, ideally the minter is a stored procedure in the db-layer, to minimize the exclusive lock duration. Potentially a second implementation might be required in sqlite3 for development (still useless for the production cases mentioned above). It might need separate code for mysql and postgres. But introduction of specific db-adapter dependencies in AF:noid seems undesirable, or at least a marked shift in the profile of the gem.

If created at a rails engine level (Sufia, CurationConcerns, etc.), the dependency profile there doesn't change (as much), but the reusability of the minter is limited.

I am leaning towards a separate gem, possibly a separate gem per DB adapter. I'd welcome some feedback from folks w/ more experience around current noid generation.