Default values on initialization #119

lewispb · 2023-07-08T13:59:01Z

I built upon what was started in #89 but attempted a simpler implementation, only setting default values upon type initialization.

Closes #66 and #89

Almost all the same tests pass, with one or two minor differences.

Todo

Update readme
Code style (see Set up RuboCop with CI #120)

…tialize * origin/main: (22 commits) Add kredis_ordered_set for OrderedSet usage in models Add a development console Bump version for 1.5.0 Fix ordered set prepend bug (rails#115) Unique list with sorted set (rails#114) Eliminating Ruby Warnings (rails#112) CI against Redis 7, Ruby 3.1, and Ruby 3.2 (rails#113) Bump version for 1.4.0 Update nokogiri for compatibility Revert "Improved version of UniqueList: OrderedSet (rails#76)" (rails#111) Add `last` to lists (rails#97) Improved version of UniqueList: OrderedSet (rails#76) Return Time objects instead of deprecated DateTime (rails#106) Fix possible deserialization of untrusted data Typecast return of Set#take (rails#105) Declare Active Model dependency (rails#107) Address LogSubscriber deprecation (rails#98) Account for time zones in DateTime serializations (rails#102) Add sample to set (rails#100) Bump version for 1.3.0 ...

test/types/enum_test.rb

* main: Run rubocop autocorrect Set up RuboCop with CI Make boolean storage more efficient Support line filtering in tests Update Readme on local development and debugging Set up ruby/debug in place of byebug for dev and test

dhh · 2023-07-24T12:02:22Z

I like this a lot. Nice simplification 👍

jeremy

Enum is a bit of an oddball re. defaults out of the enumerated set and nil defaults in particular. From HEY:

kredis_enum :status, values: %w[ waiting confirmed ], default: nil, key: …

Defaults out of the enumerated set should probably be considered a bug, with a special case for default: nil behaving how an optional: true argument would.

lib/kredis/default_values.rb

lib/kredis/types/scalar.rb

lib/kredis/attributes.rb

lib/kredis/default_values.rb

lib/kredis/types/enum.rb

test/types/enum_test.rb

…d default

lewispb · 2023-08-02T12:07:07Z

Defaults out of the enumerated set should probably be considered a bug, with a special case for nil

Thanks @jeremy, agree, and updated to raise an error in that case.

tleish · 2023-08-06T14:47:26Z

README.md

+write the specified default value.
+
+This means that using default values in a typical Rails app additional Redis calls (WATCH, EXISTS, UNWATCH) will be
+executed for each Kredis attribute with a default value read or written during a request.


Does this occur every request, or only when a value does not exist?

In the context of a Rails web request, as the Kredis object is initialized, the key is checked with WATCH, EXISTS, UNWATCH once, but then subsequent operations within the request do not incur any Redis command overhead. If no default value is specified, no additional Redis command overhead is incurred at all.

To clarify:

Scenario 1: Default NOT defined

(no additional overhead)

Scenario 2: Default defined, value NOT exists

WATCH, EXISTS, UNWATCH

Scenario 3: Default defined, value exists

(no additional overhead?)

Actually, it's:

Scenario 1: Default NOT defined

(no additional overhead)

Scenario 2: Default defined, value NOT exists

WATCH, EXISTS, write default (depending on type), UNWATCH

Scenario 3: Default defined, value exists

WATCH, EXISTS, UNWATCH

It seems a more efficient approach would be to only add extra overhead of default is defined AND value doesn't exist. In other words:

Get Value

Value doesn't exist

Execute Default Logic

This avoids extra overhead if the value has been set already.

I agree @tleish. That's exactly what we do here, but with the added WATCH / UNWATCH.

To illustrate the problem that the WATCH / UNWATCH solves check this test case:

kredis/test/types/counter_test.rb

Lines 113 to 121 in b1a818a

test "concurrent initialization with default" do

5.times.map do

Thread.new do

Kredis.counter("mycounter", default: 5).increment

end

end.each(&:join)

assert_equal 10, Kredis.counter("mycounter").value

end

Without the WATCH / UNWATCH, the value existing check is prone to a race condition.

nzwsch · 2023-10-14T09:05:40Z

I noticed that the default value cannot be set in kredis_integer in the current release version 1.5.0.

Failure/Error: kredis_integer :foo, default: 0

ArgumentError:
  unknown keyword: :default

When will kredis be released with this PR?

dhh · 2023-10-19T17:44:07Z

1.6.0 released 👍

tleish and others added 29 commits June 20, 2022 10:03

Add default proxy value

133a0e0

Add default list values

4eff947

Add default to unique_list

903f2a9

Add default to flag

a668f78

Add default to string

35dc9bf

Add default to integer

e6bcbbd

Add default to decimal

ba0092c

Add default to datetime

52b308b

Add default to float

de0e5b1

Add default proc to enum

77499ed

rename default_value method to default

46fa83a

Add default proc to set

abdf95a

Add additional unit tests

4933f6c

Add default proc to json and counter

321d33c

Add default proc to hash and boolean

411d04a

Add before_method_hook

ed3cbd8

Fix for ruby 3

1030836

refactor Kredis::Types::BeforeMethodsHook#before_methods

1bb9288

code review updates

10ceac9

merge main

1bff6bf

fix code review feedback

7e85720

Match indentation

180f57c

create custom callnx method and refactor

bf2ba58

add additional default options to set

d0146ee

move primary #set_default method to Kredis::Types::Proxying

90d9208

updated enum multi block to use standar [-1] pattern instead of .last

5c4664b

refactor init_default_in_multi to not use multi if default not defined

ddf555f

refactor multi in Kredis::Types::Proxying

ef2eacb

lewispb mentioned this pull request Jul 8, 2023

Default values for entries #89

Closed

Set default values on type initialization

6faf4d0

lewispb force-pushed the default-values-on-initialize branch from efa429b to 6faf4d0 Compare July 8, 2023 14:03

lewispb commented Jul 8, 2023

View reviewed changes

test/types/enum_test.rb Show resolved Hide resolved

lewispb added 2 commits July 13, 2023 09:51

Merge branch 'main' into default-values-on-initialize

a140434

* main: Run rubocop autocorrect Set up RuboCop with CI Make boolean storage more efficient Support line filtering in tests Update Readme on local development and debugging Set up ruby/debug in place of byebug for dev and test

Run Rubocop autocorrect over default value tests

52000bb

dhh requested a review from jeremy July 24, 2023 12:02

Document default values and organize readme

2237ecc

jeremy reviewed Aug 2, 2023

View reviewed changes

lewispb added 5 commits August 2, 2023 12:26

Support concurrent initalization with defaults

c32c663

Avoid breaking current enum functionality, allowing setting an invali…

c808076

…d default

Now we have proper concurrency, no need for scalar default nx: true

b29a895

Restore test for enum not existing when value is nil

5c5ee24

Setting an invalid enum default is a bug

cc08f25

lewispb requested a review from jeremy August 2, 2023 12:07

No need to double proxy watch

b1a818a

jeremy approved these changes Aug 5, 2023

View reviewed changes

dhh merged commit cc15c2f into rails:main Aug 6, 2023
17 checks passed

lewispb deleted the default-values-on-initialize branch August 6, 2023 10:02

tleish reviewed Aug 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default values on initialization #119

Default values on initialization #119

lewispb commented Jul 8, 2023 •

edited

Loading

dhh commented Jul 24, 2023

jeremy left a comment

lewispb commented Aug 2, 2023

tleish Aug 6, 2023

lewispb Aug 7, 2023

tleish Aug 7, 2023

lewispb Aug 7, 2023 •

edited

Loading

tleish Aug 7, 2023

lewispb Aug 8, 2023 •

edited

Loading

nzwsch commented Oct 14, 2023

dhh commented Oct 19, 2023

	test "concurrent initialization with default" do
	5.times.map do
	Thread.new do
	Kredis.counter("mycounter", default: 5).increment
	end
	end.each(&:join)

	assert_equal 10, Kredis.counter("mycounter").value
	end

Default values on initialization #119

Default values on initialization #119

Conversation

lewispb commented Jul 8, 2023 • edited Loading

dhh commented Jul 24, 2023

jeremy left a comment

Choose a reason for hiding this comment

lewispb commented Aug 2, 2023

tleish Aug 6, 2023

Choose a reason for hiding this comment

lewispb Aug 7, 2023

Choose a reason for hiding this comment

tleish Aug 7, 2023

Choose a reason for hiding this comment

Scenario 1: Default NOT defined

Scenario 2: Default defined, value NOT exists

Scenario 3: Default defined, value exists

lewispb Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Scenario 1: Default NOT defined

Scenario 2: Default defined, value NOT exists

Scenario 3: Default defined, value exists

tleish Aug 7, 2023

Choose a reason for hiding this comment

lewispb Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

nzwsch commented Oct 14, 2023

dhh commented Oct 19, 2023

lewispb commented Jul 8, 2023 •

edited

Loading

lewispb Aug 7, 2023 •

edited

Loading

lewispb Aug 8, 2023 •

edited

Loading