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

Cache if gem is indexed in Gemstash::Storage #44

Merged
merged 4 commits into from
Nov 1, 2015

Conversation

smellsblue
Copy link
Contributor

This moves whether a gem is indexed into Gemstash::Storage so we don't need to do any DB queries when bundling with a Gemfile.lock.

This also combines a migration as described in #43.

Important note: There is now a small amount of file interaction inside a database transaction. I think it might be fine as it should be pretty minimal in an infrequent codepath, but it makes me uncomfortable enough to mention it here. It is nice that failed file operations would cause a DB rollback, but is this shortsighted in some way I'm not thinking of?

FileUtils.mkpath(@folder) unless Dir.exist?(@folder)
save_file(content_filename) { @content }
save_file(content_filename) { @content } if storing == :all
save_file(properties_filename) { @properties.to_yaml }
self
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about?

def save(content, properties: nil)
  save_content(content)
  save_properties(properties) unless properties.nil?
  self
end

def update_properties(data)
  load
  save_properties(@properties.merge(data))
  self
end

def save_content(content)
  store(content_filename, content)
  @content = content
end

def save_properties(properties)
  store(properties_filename, properties.to_yam)
  @properties = properties
end

def store(filename, content)
  FileUtils.mkpath(@folder) unless Dir.exist?(@folder)
  save_file(content_filename) { content }
  self
end

I have problems with flags 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 pushed an update

@pcarranza
Copy link
Contributor

Regarding a rollback triggered by a saving failure, I don't think that it is an issue at all, I find it hard to think of a case when this will happen, and also if anyone is failing to save a yaml file probably has bigger issues than a rolled back transaction.

@smellsblue
Copy link
Contributor Author

@pcarranza well, the rollback triggered by saving failure is kinda what I want (otherwise either the DB or the file system will be out of sync, neither case is good, especially with Gemstash::GemSource::PrivateSource using the storage to determine if the gem is indexed). What I don't like is interacting with anything but the database inside a database transaction... but I think it's fine and even desirable in this case... just wondering if there might be a better way :-)

@pcarranza
Copy link
Contributor

I think it is fair enough. Not interacting would mean to do it with some kind of queuing system just registering the job and doing it later. Given the fact that this is such a small touch (a few bytes?) I wouldn't worry at all. It is quite an edge case.

@pcarranza
Copy link
Contributor

👍 or :shipit:

smellsblue added a commit that referenced this pull request Nov 1, 2015
Cache if gem is indexed in Gemstash::Storage
@smellsblue smellsblue merged commit 4c7911e into master Nov 1, 2015
@smellsblue smellsblue deleted the indexed-in-storage-properties branch November 1, 2015 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants