Skip to content
Merged

0.30.11 #1676

Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,16 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).

# [UNRELEASED] -

## Changed

- If user already have import with the same name, it will be appended with timestamp during the import process.

## Fixed

- Some types of imports were not being detected correctly and were failing to import.

# [0.30.10] - 2025-08-22

## Added
Expand Down
24 changes: 11 additions & 13 deletions app/controllers/imports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,27 +102,25 @@ def create_import_from_signed_id(signed_id)

blob = ActiveStorage::Blob.find_signed(signed_id)

import = current_user.imports.build(name: blob.filename.to_s)
import_name = generate_unique_import_name(blob.filename.to_s)
import = current_user.imports.build(name: import_name)
import.file.attach(blob)
import.source = detect_import_source(import.file) if import.source.blank?

import.save!

import
end

def detect_import_source(file_attachment)
temp_file_path = Imports::SecureFileDownloader.new(file_attachment).download_to_temp_file
def generate_unique_import_name(original_name)
return original_name unless current_user.imports.exists?(name: original_name)

Imports::SourceDetector.new_from_file_header(temp_file_path).detect_source
rescue StandardError => e
Rails.logger.warn "Failed to auto-detect import source for #{file_attachment.filename}: #{e.message}"
nil
ensure
# Cleanup temp file
if temp_file_path && File.exist?(temp_file_path)
File.unlink(temp_file_path)
end
# Extract filename and extension
basename = File.basename(original_name, File.extname(original_name))
extension = File.extname(original_name)

# Add current datetime
timestamp = Time.current.strftime('%Y%m%d_%H%M%S')
"#{basename}_#{timestamp}#{extension}"
end
Comment on lines +114 to 124
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

❓ Verification inconclusive

Strengthen uniqueness: second-level timestamp can collide; also trim trailing whitespace

If a user uploads multiple files with the same original name within the same second, both will receive the same timestamped name, causing collisions. Use microseconds and re-check in a loop (or add a short counter suffix) to guarantee uniqueness. Also, RuboCop flagged trailing whitespace (Line 120/121).

Apply this diff to harden uniqueness and remove trailing spaces:

-  def generate_unique_import_name(original_name)
-    return original_name unless current_user.imports.exists?(name: original_name)
-
-    # Extract filename and extension
-    basename = File.basename(original_name, File.extname(original_name))
-    extension = File.extname(original_name)
-    
-    # Add current datetime
-    timestamp = Time.current.strftime('%Y%m%d_%H%M%S')
-    "#{basename}_#{timestamp}#{extension}"
-  end
+  def generate_unique_import_name(original_name)
+    # Fast path if unused
+    return original_name unless current_user.imports.exists?(name: original_name)
+
+    base = File.basename(original_name, File.extname(original_name))
+    ext  = File.extname(original_name)
+
+    # Use microsecond precision and retry if needed to guarantee uniqueness
+    timestamp = Time.current.utc.strftime('%Y%m%d_%H%M%S%6N')
+    candidate = "#{base}_#{timestamp}#{ext}"
+    i = 1
+    while current_user.imports.exists?(name: candidate)
+      candidate = "#{base}_#{timestamp}_#{i}#{ext}"
+      i += 1
+    end
+    candidate
+  end

If there’s a unique DB index on imports.name, we’re covered at the database layer too. Check with:


🏁 Script executed:

#!/bin/bash
rg -nP --type=ruby -C2 'add_index\s+:imports\b.*\b:name\b.*\bunique:\s*true' db

Length of output: 81


🏁 Script executed:

#!/bin/bash
# Search for a unique DB index on imports.name in migrations
rg -nP --type=ruby -C2 'add_index\s+:imports\b.*\b:name\b.*unique:\s*true' db/migrate

# Search for inline t.index declarations in migrations
rg -nP --type=ruby -C2 't\.index\s+:name\b.*unique:\s*true' db/migrate

# Verify schema.rb for any index definitions on imports.name
if [ -f db/schema.rb ]; then
  rg -nP 'add_index\s+"imports", \["name"\], unique: true' db/schema.rb
  rg -nP 't\.index\s+:name, unique: true' db/schema.rb
fi

Length of output: 313


Ensure true name uniqueness and clean up whitespace
I didn’t find a unique database index on imports.name, so name collisions could still occur if two imports are created within the same microsecond. Please:

  • Harden the Ruby helper to use microsecond precision with a retry counter fallback for guaranteed uniqueness.
  • Remove the RuboCop-flagged trailing whitespace.
  • Add a unique index on imports.name at the database level (e.g. via a Rails migration) to enforce this constraint.

Apply these changes:

   # app/controllers/imports_controller.rb

-  def generate_unique_import_name(original_name)
-    return original_name unless current_user.imports.exists?(name: original_name)
-
-    # Extract filename and extension
-    basename = File.basename(original_name, File.extname(original_name))
-    extension = File.extname(original_name)
-    
-    # Add current datetime
-    timestamp = Time.current.strftime('%Y%m%d_%H%M%S')
-    "#{basename}_#{timestamp}#{extension}"
-  end
+  def generate_unique_import_name(original_name)
+    # Fast path if the name isn’t taken
+    return original_name unless current_user.imports.exists?(name: original_name)
+
+    base      = File.basename(original_name, File.extname(original_name))
+    ext       = File.extname(original_name)
+    timestamp = Time.current.utc.strftime('%Y%m%d_%H%M%S%6N')
+
+    candidate = "#{base}_#{timestamp}#{ext}"
+    counter   = 1
+    while current_user.imports.exists?(name: candidate)
+      candidate = "#{base}_#{timestamp}_#{counter}#{ext}"
+      counter += 1
+    end
+
+    candidate
+  end

And add a migration such as:

class AddUniqueIndexToImportsName < ActiveRecord::Migration[6.1]
  def change
    add_index :imports, :name, unique: true
  end
end

This combination guarantees both application-level and database-level uniqueness.

🧰 Tools
🪛 RuboCop (1.76.1)

[convention] 120-120: Trailing whitespace detected.

(Layout/TrailingWhitespace)

🤖 Prompt for AI Agents
In app/controllers/imports_controller.rb around lines 114 to 124, the helper
generate_unique_import_name can still collide and contains trailing whitespace;
update it to strip any surrounding whitespace from original_name, use
Time.current.strftime with microsecond precision (e.g. '%Y%m%d_%H%M%S_%6N') and
attempt a looped retry with an incrementing counter appended if a uniqueness
check still collides, returning a guaranteed-unique name; remove the trailing
whitespace in the file; and add a Rails migration to add a unique database index
on imports.name (e.g. add_index :imports, :name, unique: true) so uniqueness is
enforced at the DB level.


def validate_points_limit
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/settings/background_jobs_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class Settings::BackgroundJobsController < ApplicationController
%w[start_immich_import start_photoprism_import].include?(params[:job_name])
}

def index;end
def index; end

def create
EnqueueBackgroundJob.perform_later(params[:job_name], current_user.id)
Expand Down
2 changes: 1 addition & 1 deletion app/models/import.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class Import < ApplicationRecord
google_semantic_history: 0, owntracks: 1, google_records: 2,
google_phone_takeout: 3, gpx: 4, immich_api: 5, geojson: 6, photoprism_api: 7,
user_data_archive: 8
}
}, allow_nil: true
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Enum now allows nil; verify downstream assumptions and queries

Allowing source to be nil matches the migration and tests. Please double-check:

  • Any validations, scopes, or queries that previously assumed presence (e.g., where(source: ...) without handling nil, or UI filters) still behave correctly.
  • Background jobs and services that branch on source tolerate nil (they should, given should_detect_source? gating in the service).

Run this quick scan to find potential assumptions:


🏁 Script executed:

#!/bin/bash
# Find validations or code paths that assume presence of `source`
rg -n -C2 --type ruby '\bvalidates\s+:source|\bsource\.\w+\?|where\([^)]*source:|\bgroup\(:source\)|pluck\(:source\)' -g '!**/spec/**'

# Find places that compare or branch on source without nil guards
rg -n -C2 --type ruby '\bif\s+.*source\b|\bcase\s+.*source\b'

Length of output: 2961


Handle nil source in downstream code paths

The source enum now allows nil; several services, serializers, and helpers assume a non-nil value and will raise or return undesirable nils. Please address the following critical locations:

  • app/services/imports/watcher.rb (around line 73)
    Calling source.to_sym on nil triggers a NoMethodError. Add a nil guard (e.g. case source&.to_sym) or early return, and define a fallback MIME type.

  • app/services/imports/create.rb (importer method at lines 51–54)
    case source.to_s yields "" for nil, so no branch matches and the method returns nil. Either provide a default importer or raise a clear error for unsupported sources.

  • app/services/photos/thumbnail.rb (around lines 31–33 and 49–50)
    case source and if source == 'immich' silently skip for nil, leading to nil URLs or missing headers. Ensure there’s a sensible default path or explicit handling for a nil source.

  • app/serializers/api/photo_serializer.rb (around lines 66–68)
    orientation returns nil when source is nil. Supply a default orientation (e.g. 'landscape') or guard against nil.

  • app/helpers/trips_helper.rb (around lines 19–21)
    photo_search_url returns nil for a nil source, breaking UI filters. Handle the nil case with a default search URL or an error.

These code paths must be updated to either guard against nil or explicitly handle it to avoid runtime errors and ensure consistent behavior.

🤖 Prompt for AI Agents
In app/models/import.rb around line 24, allow_nil on the source enum exposes nil
downstream; update the listed callers to guard or handle nil: in
app/services/imports/watcher.rb (≈line 73) use a nil-safe check (e.g. case
source&.to_sym or early return) and provide a fallback MIME type; in
app/services/imports/create.rb (importer method lines 51–54) ensure nil maps to
a default importer or raise a clear "unsupported source" error instead of
returning nil; in app/services/photos/thumbnail.rb (≈lines 31–33 and 49–50) add
explicit handling for nil source (default thumbnail path or headers) so
URLs/headers are not nil; in app/serializers/api/photo_serializer.rb (≈lines
66–68) return a default orientation like 'landscape' when source is nil; in
app/helpers/trips_helper.rb (≈lines 19–21) make photo_search_url return a
sensible default URL or fallback filter when source is nil. Ensure each change
either short-circuits on nil with a clear fallback or raises a descriptive error
so downstream code never operates on an unexpected nil source.


def process!
if user_data_archive?
Expand Down
14 changes: 13 additions & 1 deletion app/services/imports/create.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,13 @@ def call

temp_file_path = Imports::SecureFileDownloader.new(import.file).download_to_temp_file

source = import.source.presence || detect_source_from_file(temp_file_path)
source = if import.source.nil? || should_detect_source?
detect_source_from_file(temp_file_path)
else
import.source
end

import.update!(source: source)
Comment on lines +19 to +25
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Avoid overriding explicit sources and lazily download only when detection is needed.

  • Current condition re-detects for every non-API source, potentially overriding a deliberately set import.source. Confirm this is intended.
  • The file is downloaded before deciding whether detection is needed; for API sources this can be unnecessary or even fail if no file is attached.

Replace this block to defer downloading until needed and not override when detection is skipped:

-    source = if import.source.nil? || should_detect_source?
-               detect_source_from_file(temp_file_path)
-             else
-               import.source
-             end
-
-    import.update!(source: source)
+    source = import.source
+    temp_file_path = nil
+    if source.nil? || should_detect_source?
+      temp_file_path = Imports::SecureFileDownloader.new(import.file).download_to_temp_file
+      source = detect_source_from_file(temp_file_path)
+    end
+    import.update!(source: source)

Note: With this change you should remove the unconditional download earlier in the method (Line 17) so the file is not fetched for API imports.

Please verify Photos::Importer accepts a nil temp_file_path:


🏁 Script executed:

#!/bin/bash
# Locate Photos::Importer initializer to confirm file arg is optional or unused.
rg -nP --type rb -C3 'class\s+Photos::Importer\b|def\s+initialize\(' app

Length of output: 101


🏁 Script executed:

#!/bin/bash
# Locate Photos::Importer class definition
rg -n 'class Photos::Importer' -g '*.rb' app

# Inspect the initialize method signature in that file
rg -n -C3 'def initialize' -g '*.rb' app

Length of output: 46105


Defer file download and preserve explicit sources

The current implementation always downloads the import file before checking if source detection is needed and may overwrite an explicitly set import.source. To avoid unnecessary downloads (especially for API imports without attachments) and to respect deliberate source assignments, update the code in app/services/imports/create.rb as follows:

• Only download when detection is required (i.e., when import.source is nil or should_detect_source? returns true).
• Preserve an existing import.source when detection is skipped.
• Remove the earlier unconditional download (around Line 17) so API imports without files won’t trigger a fetch.
• Note: Photos::Importer#initialize(import, user_id, file_path = nil) already accepts a nil file_path, so deferring download will not break its usage.

Locations to update:

  • app/services/imports/create.rb (around Lines 17–25)

Proposed diff:

-    # existing unconditional download at Line 17 – remove this:
-    temp_file_path = Imports::SecureFileDownloader.new(import.file).download_to_temp_file
-
-    source = if import.source.nil? || should_detect_source?
-               detect_source_from_file(temp_file_path)
-             else
-               import.source
-             end
-
-    import.update!(source: source)
+    source       = import.source
+    temp_file_path = nil
+
+    if source.nil? || should_detect_source?
+      # Only download when we need to detect
+      temp_file_path = Imports::SecureFileDownloader.new(import.file).download_to_temp_file
+      source = detect_source_from_file(temp_file_path)
+    end
+
+    import.update!(source: source)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
source = if import.source.nil? || should_detect_source?
detect_source_from_file(temp_file_path)
else
import.source
end
import.update!(source: source)
source = import.source
temp_file_path = nil
if source.nil? || should_detect_source?
# Only download when we need to detect
temp_file_path = Imports::SecureFileDownloader.new(import.file).download_to_temp_file
source = detect_source_from_file(temp_file_path)
end
import.update!(source: source)
🤖 Prompt for AI Agents
In app/services/imports/create.rb around lines 17 to 25, remove the
unconditional file download and instead only download the file when detection is
required (i.e., when import.source.nil? || should_detect_source? is true); if
detection is required, download to temp_file_path, run
detect_source_from_file(temp_file_path) and set source from that detection,
otherwise preserve the existing import.source (do not overwrite it); ensure
import.update!(source: source) is only called with the selected source and that
Photos::Importer is invoked with file_path set to the temp path only when a file
was downloaded (pass nil otherwise).

importer(source).new(import, user.id, temp_file_path).call

schedule_stats_creating(user.id)
Expand Down Expand Up @@ -90,8 +96,14 @@ def create_import_failed_notification(import, user, error)
).call
end

def should_detect_source?
# Don't override API-based sources that can't be reliably detected
!%w[immich_api photoprism_api].include?(import.source)
end

def detect_source_from_file(temp_file_path)
detector = Imports::SourceDetector.new_from_file_header(temp_file_path)

detector.detect_source!
end

Expand Down
12 changes: 6 additions & 6 deletions app/services/imports/source_detector.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,10 @@ def initialize(file_content, filename = nil, file_path = nil)

def self.new_from_file_header(file_path)
filename = File.basename(file_path)

# For detection, read only first 2KB to optimize performance
header_content = File.open(file_path, 'rb') { |f| f.read(2048) }

new(header_content, filename, file_path)
end

Expand Down Expand Up @@ -103,15 +103,15 @@ def gpx_file?

# Must have .gpx extension AND contain GPX XML structure
return false unless filename.downcase.end_with?('.gpx')

# Check content for GPX structure
content_to_check = if file_path && File.exist?(file_path)
# Read first 1KB for GPX detection
File.open(file_path, 'rb') { |f| f.read(1024) }
else
file_content
end

content_to_check.strip.start_with?('<?xml') && content_to_check.include?('<gpx')
end

Expand All @@ -120,15 +120,15 @@ def owntracks_file?

# Check for .rec extension first (fastest check)
return true if filename.downcase.end_with?('.rec')

# Check for specific OwnTracks line format in content
content_to_check = if file_path && File.exist?(file_path)
# For OwnTracks, read first few lines only
File.open(file_path, 'r') { |f| f.read(2048) }
else
file_content
end

content_to_check.lines.any? { |line| line.include?('"_type":"location"') }
end

Expand Down
9 changes: 5 additions & 4 deletions app/views/imports/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
</div>
</div>
<div class="overflow-x-auto">
<table class="table overflow-x-auto">
<table class="table table-zebra overflow-x-auto">
<thead>
<tr>
<th>Name</th>
Expand All @@ -55,7 +55,8 @@
<% @imports.each do |import| %>
<tr data-import-id="<%= import.id %>"
id="import-<%= import.id %>"
data-points-total="<%= import.processed %>">
data-points-total="<%= import.processed %>"
class="hover">
<td>
<%= link_to import.name, import, class: 'underline hover:no-underline' %>
(<%= import.source %>)
Expand All @@ -72,9 +73,9 @@
<td><%= human_datetime(import.created_at) %></td>
<td class="whitespace-nowrap">
<% if import.file.present? %>
<%= link_to 'Download', rails_blob_path(import.file, disposition: 'attachment'), class: "px-4 py-2 bg-blue-500 text-white rounded-md", download: import.name %>
<%= link_to 'Download', rails_blob_path(import.file, disposition: 'attachment'), class: "btn btn-outline btn-sm btn-info", download: import.name %>
<% end %>
<%= link_to 'Delete', import, data: { confirm: "Are you sure?", turbo_confirm: "Are you sure?", turbo_method: :delete }, method: :delete, class: "px-4 py-2 bg-red-500 text-white rounded-md" %>
<%= link_to 'Delete', import, data: { confirm: "Are you sure?", turbo_confirm: "Are you sure?", turbo_method: :delete }, method: :delete, class: "btn btn-outline btn-sm btn-error" %>
</td>
</tr>
<% end %>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class RemoveDefaultFromImportsSource < ActiveRecord::Migration[8.0]
def change
change_column_default :imports, :source, from: 0, to: nil
end
end
4 changes: 2 additions & 2 deletions db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion spec/factories/imports.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
factory :import do
user
sequence(:name) { |n| "owntracks_export_#{n}.json" }
source { Import.sources[:owntracks] }
# source { Import.sources[:owntracks] }

trait :with_points do
after(:create) do |import|
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// This file contains 3 doubles

{
"semanticSegments": [
{
Expand Down
2 changes: 1 addition & 1 deletion spec/services/google_maps/phone_takeout_importer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

context 'when file content is an object' do
# This file contains 3 duplicates
let(:file_path) { Rails.root.join('spec/fixtures/files/google/phone-takeout.json') }
let(:file_path) { Rails.root.join('spec/fixtures/files/google/phone-takeout_w_3_duplicates.json') }
let(:file) { Rack::Test::UploadedFile.new(file_path, 'application/json') }
let(:import) { create(:import, user:, name: 'phone_takeout.json', file:) }

Expand Down
11 changes: 9 additions & 2 deletions spec/services/imports/create_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@
expect(import.reload.status).to eq('processing').or eq('completed')
end

it 'updates the import source' do
service.call

expect(import.reload.source).to eq('owntracks')
end

context 'when import succeeds' do
it 'sets status to completed' do
service.call
Expand Down Expand Up @@ -63,10 +69,10 @@

context 'when source is google_phone_takeout' do
let(:import) { create(:import, source: 'google_phone_takeout') }
let(:file_path) { Rails.root.join('spec/fixtures/files/google/phone-takeout.json') }
let(:file_path) { Rails.root.join('spec/fixtures/files/google/phone-takeout_w_3_duplicates.json') }

before do
import.file.attach(io: File.open(file_path), filename: 'phone-takeout.json',
import.file.attach(io: File.open(file_path), filename: 'phone-takeout_w_3_duplicates.json',
content_type: 'application/json')
end

Expand Down Expand Up @@ -193,6 +199,7 @@
it 'calls the Photos::Importer' do
expect(Photos::Importer).to \
receive(:new).with(import, user.id, kind_of(String)).and_return(double(call: true))

service.call
end
end
Expand Down
4 changes: 2 additions & 2 deletions spec/services/imports/source_detector_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
end

context 'with Google Phone Takeout format' do
let(:file_content) { file_fixture('google/phone-takeout.json').read }
let(:file_content) { file_fixture('google/phone-takeout_w_3_duplicates.json').read }

it 'detects google_phone_takeout format' do
expect(detector.detect_source).to eq(:google_phone_takeout)
Expand Down Expand Up @@ -131,7 +131,7 @@

it 'can detect source efficiently from file' do
detector = described_class.new_from_file_header(fixture_path)

# Verify it can detect correctly using file-based approach
expect(detector.detect_source).to eq(:google_records)
end
Expand Down
2 changes: 1 addition & 1 deletion spec/services/users/import_data/imports_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@
created_imports = user.imports.pluck(:name, :source)
expect(created_imports).to contain_exactly(
['Valid Import', 'owntracks'],
['Missing Source Import', 'google_semantic_history']
['Missing Source Import', nil]
)
end

Expand Down