Fix record history export #4
Conversation
Previously we were erroneously testing to see if a value was a valid ruby key. This returned true even when the value was not a valid ruby key e.g.
```
'A's Fake Name': {
```
```
Ghtc_Hyuik_1[1]_160x160: {
```
I've change this behavior so that all keys are wrapped in single quotes and, if any single quotes exist in the key string they are escaped e.g.
```
'A\'s Fake Name': {
```
```
'Ghtc_Hyuik_1[1]_160x160': {
```
| def key_to_ruby(key) | ||
| valid_key?(key) ? key : "'#{key}'" | ||
| "'#{ key.gsub(/'/, "\\'") }'" |
There was a problem hiding this comment.
If you receive an integer you will get an exception. so you can keep the validation and add your escape to those that are not valid:
def key_to_ruby(key)
return key if valid_key?(key)
"'#{ key to_s.gsub(/'/, "\\'") }'"
end
There was a problem hiding this comment.
I wouldn't want to rely on any valid_key? method for this, everything we're being passed should be a valid key given that it comes from an existing ruby key: value pair afaik.
I would instead just coerce everything to string before applying the .gsub.
On some installations we can see upwards of 5GB of attachment data. When we try and load all of this into rails at once, depending on the available machine memory, we can see OOM exceptions which can bring everything down! Yay. We're injecting a manual GC every 100 attachment loads which will drastically reduce the lifetime of the attachment objects.
coerce all potential keys into strings before processing them
| def key_to_ruby(key) | ||
| valid_key?(key) ? key : "'#{key}'" | ||
| "'#{ key.to_s.gsub(/'/, "\\'") }'" |
There was a problem hiding this comment.
we want to keep valid_key? method call here, in order to avoid unnecesary conversion of a valid key
There was a problem hiding this comment.
This method is also changed in this PR https://github.com/primeroIMS/primero-v2-migration/pull/4/files
| @@ -18,7 +18,7 @@ def exporters | |||
| timestamp = DateTime.now.strftime('%Y%m%d%H%M%S') | |||
| export_dir = "record-data-files-#{timestamp}" | |||
| exporters.each do |exporter| | |||
| data_exporter = Object.const_get(exporter).new(export_dir: export_dir, batch_size: 250) | |||
| data_exporter = Object.const_get(exporter).new(export_dir: export_dir) | |||
There was a problem hiding this comment.
Keep 250 batch_size and overwrite in attachment_exporter
| @@ -18,7 +18,7 @@ def exporters | |||
| timestamp = DateTime.now.strftime('%Y%m%d%H%M%S') | |||
| export_dir = "record-data-files-#{timestamp}" | |||
| exporters.each do |exporter| | |||
| data_exporter = Object.const_get(exporter).new(export_dir: export_dir, batch_size: 250) | |||
| data_exporter = Object.const_get(exporter).new(export_dir: export_dir) | |||
There was a problem hiding this comment.
[this change is not related with record history export, please create a new pr]
| @@ -1,6 +1,7 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| require_relative('data_exporter.rb') | |||
| require 'erb' | |||
There was a problem hiding this comment.
[these changes is not related with record history export, please create a new pr]
| initialize_script_for_attachment(folder_to_save, type, sufix) | ||
|
|
||
| attachments = [] | ||
| data_object_names.each do |type| |
There was a problem hiding this comment.
this is a reduce/inject to avoid initialize array, please create a new method
| end | ||
|
|
||
| @output.puts "# force ruby to gargabe collect here as attachmented files can build up in memory!" | ||
| @output.puts "GC.start" |
There was a problem hiding this comment.
GC will be enable for every iteration. is not need to stop manually?
|
We are holding off on merging this. The main performance motive for this PR has been addressed elsewhere. |
No description provided.