Skip to content

Commit aae569e

Browse files
Copilotdamacus
Copilot
and
damacus
committed
Fix pg_ident to allow multiple mappings per map name
Co-authored-by: damacus <[email protected]>
1 parent 52076e2 commit aae569e

File tree

3 files changed

+227
-23
lines changed

3 files changed

+227
-23
lines changed

‎libraries/ident.rb‎

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -79,21 +79,31 @@ def initialize
7979
def add(entry)
8080
raise unless entry.is_a?(PgIdentFileEntry)
8181

82-
return false if entry?(entry.map_name)
82+
return false if include?(entry)
8383

8484
@entries.push(entry)
8585

8686
sort!
8787
end
8888

89-
def entry(map_name)
90-
entry = @entries.filter { |e| e.map_name.eql?(map_name) }
89+
def entry(map_name, system_username = nil, database_username = nil)
90+
entries = @entries.filter { |e| e.map_name.eql?(map_name) }
9191

92-
return if nil_or_empty?(entry)
92+
return if nil_or_empty?(entries)
9393

94-
raise PgIdentFileDuplicateEntry, "Duplicate entries found for #{map_name}" unless entry.one?
94+
# If specific system_username and database_username are provided, find exact match
95+
if system_username && database_username
96+
entry = entries.filter { |e| e.system_username.eql?(system_username) && e.database_username.eql?(database_username) }
97+
return entry.first if entry.one?
98+
return
99+
end
100+
101+
# If only map_name is provided and there's only one entry, return it
102+
return entries.first if entries.one?
95103

96-
entry.pop
104+
# If multiple entries exist for the same map_name but no specific system/database username provided
105+
# This is for backward compatibility - return the first one but don't raise an error
106+
entries.first
97107
end
98108

99109
def entry?(map_name)
@@ -103,7 +113,7 @@ def entry?(map_name)
103113
def include?(entry)
104114
raise unless entry.is_a?(PgIdentFileEntry)
105115

106-
@entries.any? { |e| e.map_name.eql?(entry.map_name) }
116+
@entries.any? { |e| e.eql?(entry) }
107117
end
108118

109119
def read!(file = 'pg_ident.conf', sort: true)
@@ -123,14 +133,13 @@ def read!(file = 'pg_ident.conf', sort: true)
123133
def remove(entry)
124134
raise unless entry.is_a?(PgIdentFileEntry) || entry.is_a?(String)
125135

126-
remove_name = case entry
127-
when PgIdentFileEntry
128-
entry.map_name
129-
when String
130-
entry
131-
end
132-
133-
@entries.reject! { |e| e.map_name.eql?(remove_name) }
136+
case entry
137+
when PgIdentFileEntry
138+
@entries.reject! { |e| e.eql?(entry) }
139+
when String
140+
# For backward compatibility, remove all entries with this map_name
141+
@entries.reject! { |e| e.map_name.eql?(entry) }
142+
end
134143
end
135144

136145
def sort!
@@ -202,7 +211,7 @@ def to_s
202211
def eql?(entry)
203212
return false unless self.class.eql?(entry.class)
204213

205-
return true if self.class.const_get(:ENTRY_FIELDS).all? { |field| send(field).eql?(entry.send(field)) }
214+
return true if %i(map_name system_username database_username).all? { |field| send(field).eql?(entry.send(field)) }
206215

207216
false
208217
end

‎resources/ident.rb‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,9 @@
5454

5555
current_value_does_not_exist! unless ident_file.entry?(new_resource.map_name)
5656

57-
entry = ident_file.entry(new_resource.map_name)
57+
entry = ident_file.entry(new_resource.map_name, new_resource.system_username, new_resource.database_username)
58+
current_value_does_not_exist! unless entry
59+
5860
%i(map_name system_username database_username comment).each { |p| send(p, entry.send(p)) }
5961
end
6062

@@ -65,7 +67,7 @@
6567
action :create do
6668
converge_if_changed do
6769
config_resource_init
68-
entry = config_resource.variables[:pg_ident].entry(new_resource.map_name)
70+
entry = config_resource.variables[:pg_ident].entry(new_resource.map_name, new_resource.system_username, new_resource.database_username)
6971

7072
if nil_or_empty?(entry)
7173
resource_properties = %i(map_name system_username database_username comment).map { |p| [ p, new_resource.send(p) ] }.to_h.compact
@@ -80,9 +82,9 @@
8082
action :update do
8183
converge_if_changed(:system_username, :database_username, :comment) do
8284
config_resource_init
83-
entry = config_resource.variables[:pg_ident].entry(new_resource.map_name)
85+
entry = config_resource.variables[:pg_ident].entry(new_resource.map_name, new_resource.system_username, new_resource.database_username)
8486

85-
raise Chef::Exceptions::CurrentValueDoesNotExist, "Cannot update ident entry for '#{new_resource.map_name}' as it does not exist" if nil_or_empty?(entry)
87+
raise Chef::Exceptions::CurrentValueDoesNotExist, "Cannot update ident entry for '#{new_resource.map_name}' with system_username '#{new_resource.system_username}' and database_username '#{new_resource.database_username}' as it does not exist" if nil_or_empty?(entry)
8688

8789
entry.update(system_username: new_resource.system_username, database_username: new_resource.database_username, comment: new_resource.comment)
8890
end
@@ -91,7 +93,14 @@
9193
action :delete do
9294
config_resource_init
9395

94-
converge_by("Remove ident entry with map_name: #{new_resource.map_name}") do
95-
config_resource.variables[:pg_ident].remove(new_resource.map_name)
96-
end if config_resource.variables[:pg_ident].entry?(new_resource.map_name)
96+
# Create an entry object to match for removal
97+
entry_to_remove = PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new(
98+
map_name: new_resource.map_name,
99+
system_username: new_resource.system_username,
100+
database_username: new_resource.database_username
101+
)
102+
103+
converge_by("Remove ident entry with map_name: #{new_resource.map_name}, system_username: #{new_resource.system_username}, database_username: #{new_resource.database_username}") do
104+
config_resource.variables[:pg_ident].remove(entry_to_remove)
105+
end if config_resource.variables[:pg_ident].include?(entry_to_remove)
97106
end

‎spec/libraries/ident_spec.rb‎

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
require 'spec_helper'
2+
require_relative '../../libraries/ident'
3+
4+
RSpec.describe PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFile do
5+
let(:ident_file) { described_class.new }
6+
7+
describe '#add' do
8+
let(:entry1) do
9+
PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new(
10+
map_name: 'testmap',
11+
system_username: 'user1',
12+
database_username: 'dbuser1'
13+
)
14+
end
15+
16+
let(:entry2) do
17+
PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new(
18+
map_name: 'testmap',
19+
system_username: 'user2',
20+
database_username: 'dbuser2'
21+
)
22+
end
23+
24+
let(:duplicate_entry) do
25+
PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new(
26+
map_name: 'testmap',
27+
system_username: 'user1',
28+
database_username: 'dbuser1'
29+
)
30+
end
31+
32+
it 'allows multiple entries with the same map_name but different system/database usernames' do
33+
result1 = ident_file.add(entry1)
34+
result2 = ident_file.add(entry2)
35+
36+
expect(result1).to be_truthy
37+
expect(result2).to be_truthy
38+
expect(ident_file.entries.size).to eq(2)
39+
end
40+
41+
it 'prevents adding duplicate entries' do
42+
ident_file.add(entry1)
43+
result = ident_file.add(duplicate_entry)
44+
45+
expect(result).to be_falsy
46+
expect(ident_file.entries.size).to eq(1)
47+
end
48+
end
49+
50+
describe '#entry' do
51+
let(:entry1) do
52+
PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new(
53+
map_name: 'testmap',
54+
system_username: 'user1',
55+
database_username: 'dbuser1'
56+
)
57+
end
58+
59+
let(:entry2) do
60+
PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new(
61+
map_name: 'testmap',
62+
system_username: 'user2',
63+
database_username: 'dbuser2'
64+
)
65+
end
66+
67+
before do
68+
ident_file.add(entry1)
69+
ident_file.add(entry2)
70+
end
71+
72+
it 'returns specific entry when all parameters are provided' do
73+
result = ident_file.entry('testmap', 'user1', 'dbuser1')
74+
expect(result).to eq(entry1)
75+
76+
result = ident_file.entry('testmap', 'user2', 'dbuser2')
77+
expect(result).to eq(entry2)
78+
end
79+
80+
it 'returns first entry when only map_name is provided and multiple entries exist' do
81+
result = ident_file.entry('testmap')
82+
expect(result).to be_a(PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry)
83+
expect(result.map_name).to eq('testmap')
84+
end
85+
86+
it 'returns nil when no matching entry is found' do
87+
result = ident_file.entry('testmap', 'nonexistent', 'user')
88+
expect(result).to be_nil
89+
end
90+
end
91+
92+
describe '#include?' do
93+
let(:entry) do
94+
PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new(
95+
map_name: 'testmap',
96+
system_username: 'user1',
97+
database_username: 'dbuser1'
98+
)
99+
end
100+
101+
let(:different_entry) do
102+
PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new(
103+
map_name: 'testmap',
104+
system_username: 'user2',
105+
database_username: 'dbuser2'
106+
)
107+
end
108+
109+
it 'returns true for existing entries' do
110+
ident_file.add(entry)
111+
expect(ident_file.include?(entry)).to be_truthy
112+
end
113+
114+
it 'returns false for non-existing entries' do
115+
ident_file.add(entry)
116+
expect(ident_file.include?(different_entry)).to be_falsy
117+
end
118+
end
119+
120+
describe 'issue scenario' do
121+
# Test the specific scenario from GitHub issue #787
122+
it 'allows multiple mappings per map name as described in the issue' do
123+
entry1 = PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new(
124+
map_name: 'someuser_postgres',
125+
system_username: 'someuser',
126+
database_username: 'postgres'
127+
)
128+
129+
entry2 = PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new(
130+
map_name: 'someuser_postgres',
131+
system_username: 'postgres',
132+
database_username: 'postgres'
133+
)
134+
135+
result1 = ident_file.add(entry1)
136+
result2 = ident_file.add(entry2)
137+
138+
expect(result1).to be_truthy
139+
expect(result2).to be_truthy
140+
expect(ident_file.entries.size).to eq(2)
141+
142+
# Verify both entries can be retrieved
143+
retrieved1 = ident_file.entry('someuser_postgres', 'someuser', 'postgres')
144+
retrieved2 = ident_file.entry('someuser_postgres', 'postgres', 'postgres')
145+
146+
expect(retrieved1).to eq(entry1)
147+
expect(retrieved2).to eq(entry2)
148+
end
149+
end
150+
end
151+
152+
RSpec.describe PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry do
153+
describe '#eql?' do
154+
let(:entry1) do
155+
described_class.new(
156+
map_name: 'testmap',
157+
system_username: 'user1',
158+
database_username: 'dbuser1'
159+
)
160+
end
161+
162+
let(:entry2) do
163+
described_class.new(
164+
map_name: 'testmap',
165+
system_username: 'user1',
166+
database_username: 'dbuser1'
167+
)
168+
end
169+
170+
let(:entry3) do
171+
described_class.new(
172+
map_name: 'testmap',
173+
system_username: 'user2',
174+
database_username: 'dbuser1'
175+
)
176+
end
177+
178+
it 'returns true for entries with same map_name, system_username, and database_username' do
179+
expect(entry1.eql?(entry2)).to be_truthy
180+
end
181+
182+
it 'returns false for entries with different system_username' do
183+
expect(entry1.eql?(entry3)).to be_falsy
184+
end
185+
end
186+
end

0 commit comments

Comments
 (0)