Fix HasOneSql imported field update support by mvorisek · Pull Request #1038 · atk4/data

mvorisek · 2022-07-24T23:38:45Z

fixes #929
closes #933
fixes atk4/filestore#26

Lines 35 to 50 in 6657685

    
                   $this->onHookToOurModel($ourModel, Model::HOOK_BEFORE_SAVE, function (Model $ourModel) use ($fieldName, $theirFieldIsTitle, $theirFieldName) { 
        
                       // if field is changed, but reference ID field (our_field) 
        
                       // is not changed, then update reference ID field value 
        
                       if ($ourModel->isDirty($fieldName) && !$ourModel->isDirty($this->our_field)) { 
        
                           $theirModel = $this->createTheirModel(); 
        
                           if ($theirFieldIsTitle) { 
        
                               $theirFieldName = $theirModel->title_field; 
        
                           } 
        
                           $theirModel->addCondition($theirFieldName, $ourModel->get($fieldName)); 
        
                           $ourModel->set($this->getOurFieldName(), $theirModel->loadOne()->getId()); 
        
                           if (!$theirFieldIsTitle) { // why for non-title only? 
        
                               $ourModel->_unset($fieldName); 
        
                           } 
        
                       } 
        
                   }, [], 20);

hook code is simply wrong.

In 5d49235 the code was intended to be fixed for changed values for different than our_field field. This caused data corruptions like atk4/filestore#26. The data corruption was hotfixed in #1037 but looking into this issue deeper, the hook should simply not be there. We cannot infer our_field values based on "imported"/"joined" fields, as these fields might not be unique and might represent totaly different relation.

The proposed fix maintains the feature for the setting the our_field value for a new record with our field equal to null, under this state, we can determine the foreign/their record based on the imported/joined field without the possibility to mismatch the our field. The relation to foreign/their record can still imply duplicates, but this is a decision by the developer.

Making the field read only after the record is saved will fix also #929.

The update issue is obvious in https://github.com/atk4/data/blob/3.1.0/tests/ReferenceSqlTest.php#L562 test. How on Earth can "first name" solely uniquely locate a foreign user record?

mvorisek force-pushed the fix_ref_link_hasone_fields branch 12 times, most recently from 485b659 to 0102881 Compare July 28, 2022 20:49

mvorisek added the BC-break label Jul 28, 2022

mvorisek force-pushed the fix_ref_link_hasone_fields branch 3 times, most recently from e34309c to a92e401 Compare July 29, 2022 10:14

mvorisek changed the title ~~Set reference link to fields added via HasOneSql~~ HasOneSql must import foreign fields as read only when our_field is set Jul 29, 2022

mvorisek changed the title ~~HasOneSql must import foreign fields as read only when our_field is set~~ HasOneSql must import foreign fields as RO when our_field is set Jul 29, 2022

mvorisek force-pushed the fix_ref_link_hasone_fields branch from a92e401 to e261c22 Compare July 29, 2022 15:51

mvorisek changed the title ~~HasOneSql must import foreign fields as RO when our_field is set~~ Fix HasOneSql imported field update support Jul 29, 2022

Fix HasOneSql imported field update support

e772085

mvorisek force-pushed the fix_ref_link_hasone_fields branch from e261c22 to e772085 Compare July 29, 2022 20:38

we should always use Lookup in ui

efaeaea

mvorisek force-pushed the fix_ref_link_hasone_fields branch from f19fc36 to 44c0238 Compare July 29, 2022 21:01

test editable

0fd39d6

mvorisek force-pushed the fix_ref_link_hasone_fields branch from 44c0238 to 0fd39d6 Compare July 29, 2022 21:10

mvorisek marked this pull request as ready for review July 29, 2022 21:10

mvorisek added bug and removed BC-break labels Jul 29, 2022

single Model insert can insert more records

cccd4ac

mvorisek added 8 commits July 30, 2022 11:49

replace tabs for string type

b55df1a

c will be always > 0 here

a05ed50

strong type some expr/query props

dd1e1ee

more strong types

a5866de

render Sqlite with backticks, prevent literal interpretation

88ae305

fix typo

1033bc2

fix TestCase::convertSqlFromSqlite

7d2abf8

improve expectException cs

c9497c1

mvorisek merged commit 0892a70 into develop Jul 30, 2022

mvorisek deleted the fix_ref_link_hasone_fields branch July 30, 2022 12:34

mvorisek mentioned this pull request Oct 19, 2022

neverPersist and hasOne added fields not working properly atk4/ui#1908

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HasOneSql imported field update support#1038

Fix HasOneSql imported field update support#1038
mvorisek merged 12 commits intodevelopfrom
fix_ref_link_hasone_fields

mvorisek commented Jul 24, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

	$this->onHookToOurModel($ourModel, Model::HOOK_BEFORE_SAVE, function (Model $ourModel) use ($fieldName, $theirFieldIsTitle, $theirFieldName) {
	// if field is changed, but reference ID field (our_field)
	// is not changed, then update reference ID field value
	if ($ourModel->isDirty($fieldName) && !$ourModel->isDirty($this->our_field)) {
	$theirModel = $this->createTheirModel();
	if ($theirFieldIsTitle) {
	$theirFieldName = $theirModel->title_field;
	}

	$theirModel->addCondition($theirFieldName, $ourModel->get($fieldName));
	$ourModel->set($this->getOurFieldName(), $theirModel->loadOne()->getId());
	if (!$theirFieldIsTitle) { // why for non-title only?
	$ourModel->_unset($fieldName);
	}
	}
	}, [], 20);

Conversation

mvorisek commented Jul 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

mvorisek commented Jul 24, 2022 •

edited

Loading