Set modified flag to true by default and make datamapper to set it to false during loading #413
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modifies the Field class to have the modified flag set to true by default (instead of false), and adds logic in DataMapper to set the modified flag to false when records are loaded from the database. This change aims to distinguish between newly constructed records and records loaded from the database. The PR also adds support for creating records with user-defined primary keys.
Key changes:
- Field's
_modifiedflag now defaults totrueinstead offalse - Added
SetModifiedState()method to DataMapper to set modified flags after loading - Modified the Create method to check if a primary key value is set before auto-assigning
- Added explicit conversion operator to Field class
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/Lightweight/DataMapper/Field.hpp | Changed default modified flag to true, added explicit conversion operator, made comparison operators constexpr |
| src/Lightweight/DataMapper/DataMapper.hpp | Added SetModifiedState() method, modified Create() to check for pre-set primary keys, added SetModifiedState() calls in all Query methods |
| src/tests/DataMapper/Entities.hpp | Added EntryWithIntPrimaryKey test entity |
| src/tests/DataMapper/CreateTests.cpp | Added test case for creating records with predefined primary keys |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… false during loading
Yaraslaut
force-pushed
the
fix/modified_flag
branch
from
034965c to
26621e6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Extra blank line. The codebase consistently uses a single blank line between template function definitions. Remove one of the blank lines.
| template <typename Record> | ||
| void ClearModifiedState(Record& record) noexcept; | ||
|
|
||
| /// Sets the modified state of the record after receiveing from the database. |
There was a problem hiding this comment.
Typo in comment: "receiveing" should be "receiving".
| /// Sets the modified state of the record after receiveing from the database. | |
| /// Sets the modified state of the record after receiving from the database. |
| auto maxId = SqlStatement { _connection }.ExecuteDirectScalar<ValueType>(std::format( | ||
| R"sql(SELECT MAX("{}") FROM "{}")sql", FieldNameOf<el>, RecordTableName<Record>)); | ||
| record.[:el:] = maxId.value_or(ValueType {}) + 1; | ||
| if(record.[:el:].Value() == ValueType {}) |
There was a problem hiding this comment.
Missing space after 'if' keyword. The codebase consistently uses 'if (' with a space, not 'if('. This should be 'if (record.[:el:].Value() == ValueType {})'.
| if(record.[:el:].Value() == ValueType {}) | |
| if (record.[:el:].Value() == ValueType {}) |
| static_assert(!std::is_const_v<Record>); | ||
| static_assert(DataMapperRecord<Record>, "Record must satisfy DataMapperRecord"); | ||
|
|
||
| Reflection::EnumerateMembers(record, []<size_t I, typename FieldType>(FieldType& field) { | ||
| if constexpr (requires { field.SetModified(false); }) | ||
| { | ||
| field.SetModified(false); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The implementation of SetNotModifiedState is identical to ClearModifiedState (lines 1788-1799). Both methods do exactly the same thing: iterate over record members and call SetModified(false). Consider either reusing ClearModifiedState or documenting why a separate method is needed. If they serve different semantic purposes, the implementation should reflect that difference, otherwise this is unnecessary code duplication.
| static_assert(!std::is_const_v<Record>); | |
| static_assert(DataMapperRecord<Record>, "Record must satisfy DataMapperRecord"); | |
| Reflection::EnumerateMembers(record, []<size_t I, typename FieldType>(FieldType& field) { | |
| if constexpr (requires { field.SetModified(false); }) | |
| { | |
| field.SetModified(false); | |
| } | |
| }); | |
| ClearModifiedState(record); |
| if (!detail::ReadSingleResult(_stmt.Connection().ServerType(), reader, *resultRecord)) | ||
| return std::nullopt; | ||
|
|
||
| if(resultRecord) |
There was a problem hiding this comment.
Here too, missing whitespace
Closes #403