Disabled storer on SaveInStorageEnabled false #4345
Conversation
…orageEnabled is false
sstanculeanu
added
type:bug
| @@ -61,3 +61,17 @@ func TestLogsRepository_GetLogsShouldErr(t *testing.T) { | |||
| require.ErrorIs(t, err, errCannotUnmarshalLog) | |||
| require.Nil(t, badLog) | |||
| } | |||
|
|
|||
| func TestLogsRepository_GetLogsShouldUseDisabled(t *testing.T) { | |||
|
|
||
| type storer struct{} | ||
|
|
||
| // NewStorer returns a new instance of this disabled storer |
There was a problem hiding this comment.
Oh, we didn't have an existing "generic" disabled storer, all the other ones were defined in their client packages. 👍 for adding it here.
node/external/logs/logsRepository.go
Outdated
| @@ -17,6 +19,10 @@ type logsRepository struct { | |||
|
|
|||
| func newLogsRepository(storageService dataRetriever.StorageService, marshaller marshal.Marshalizer) *logsRepository { | |||
| storer := storageService.GetStorer(dataRetriever.TxLogsUnit) | |||
| if check.IfNil(storer) { | |||
There was a problem hiding this comment.
It's a bit debatable if we should have used a disabledLogsFacade at a higher level, instead, when logs saving is disabled. But it's all right this way, as well 🙏
There was a problem hiding this comment.
pushed Iulian's solution which fixes this
node/external/logs/logsRepository.go
Outdated
| @@ -17,6 +19,10 @@ type logsRepository struct { | |||
|
|
|||
| func newLogsRepository(storageService dataRetriever.StorageService, marshaller marshal.Marshalizer) *logsRepository { | |||
| storer := storageService.GetStorer(dataRetriever.TxLogsUnit) | |||
| if check.IfNil(storer) { | |||
There was a problem hiding this comment.
instead of this approach, I would make the storageService.GetStorer(dataRetriever.TxLogsUnit) call return always a not-nil pointer, on all conditions. So, in StorageServiceFactory.setupLogsAndEventsStorer function, on L556 I would add a disabled storer.
This will simplify all involved code and this new if branch will no longer be necessary.
For a next PR: we might return an error on the GetStorer function whenever we've provided a key that does not contain a valid storer as to align the code to the rest of the project.
Codecov Report
@@ Coverage Diff @@
## rc/2022-july #4345 +/- ##
================================================
- Coverage 73.74% 73.70% -0.04%
================================================
Files 670 670
Lines 86388 86390 +2
================================================
- Hits 63707 63677 -30
- Misses 17908 17940 +32
Partials 4773 4773
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@@ Log scanner @@
fix_logs_repository
================================================================================
- Known Warnings 14
- New Warnings 3
- Known Errors 0
- New Errors 0
- Panics 0
================================================================================
Description of the reasoning behind the pull request (what feature was missing / how the problem was manifesting itself / what was the motive behind the refactoring)
SaveInStorageEnabledwas false, theTxLogsUnitstorer was nil, leading to panic on different api callsProposed Changes
Testing procedure
/block/by-nonce/15?withTxs=true&withLogs=trueshould not panic no matter what block is asked for, with config valuesLogsAndEvents.SaveInStorageEnabled = falseandDbLookupExtensions.Enabled = false