Track IsNew as property

[6pac]

I note and echo some concerns raised in #10 (it is from 2011 so I've opened this new issue!).

I tried inserting a PK into an autonumber-PK field, which is acceptable on many DBMS's.

However IsNew is coded to base the return on whether the PK is still at default value or not.
Save() calls IsNew() and, getting false, then calls Update(), which fails silently because the record does not exist.
Calling Insert() directly fails because it will not insert a value into an autonumber column.

I propose creating an IsNew property on the poco column that will track the status (IsNew=false where loaded from the DB, new otherwise). This is how SubSonic worked.
Also perhaps a CanInsertAutonum property on the providers to allow this behaviour.

Just checking this would be acceptable before I tool up a PR.

[asherber]

I haven't put together a test project for this, but what if you just set autoIncrement to false? I think in that case PP should Insert whatever you want.

Note also that while IsNew() basically just checks to see if the PK property has a default value, you can also use Exists() to check whether a non-default value PK actually exists in the db.

If neither of those meet your needs, then how would you design this new IsNew() so that it doesn't conflict with the existing one?

[6pac]

what if you just set autoIncrement to false?

True, but this is an autoincrement field. I want new records to autoincrement unless I specify a value. In practice, this is a mobile app, and for the sync operations I am importing a lot of external data (insert an external value into the autoincrement id field) while for local editing I am creating genuinely new records.

you can also use Exists()

definitely don't want to round trip to the db

then how would you design this new IsNew()

the property would replace the method, so the method could just return the property. as I mentioned, SubSonic (PetaPoco's ancestor project) did this. i still have all the code (several projects still use SS2 and SS3). PetaPoco is more lightweight, so I'd have to check that it has the capability to track this. SS also had the ability to track 'dirty' columns, so it would just generate an update to columns that had been changed, rather than all of them.

I don't want to keep banging on about SubSonic, but it was a great project and I only left it because it was too LINQ-y (a mistake IMO) and SS3 had some performance issues. PP inherited all its class templates from SS. I've hacked the templates (extensively) and the core code (somewhat) of PP based on a version from several years ago, and I'm just now coming back to the project source repo due to the T4 issues and NET Standard support. So I am keen to feed back features I see as useful.

Just out of interest, I am also a core maintainer on the Slickgrid project.

[asherber]

Okay, I see where you're going, but I have a couple of concerns. First, by redefining what IsNew() means, you're introducing a breaking change. Second, I'm not sure that the flag you're suggesting would necessarily be any more reliable than what PP has currently -- I could fill out a POCO that did not come from the db (so IsNew is true) and yet which has a PK that already exists in the db.

I'm not opposed to what you're suggesting, just trying to suss out some details.

How about a modification to Insert()? Currently I think the behavior is that if there's an auto-increment PK, don't include that property in the SQL. What if that were changed so that PP does send the property if it has a non-default value? I think that would accomplish what you're after, though it's also a breaking change. How about a new method to implement this behavior, something like InsertWithPrimaryKey()?

[iadaz]

An InsertWithPrimaryKey() function would get my vote, it's just so less ambiguous than inferring behaviour from POCO properties. If the property name doesn't explicitly advertise what it does it's bound to get misused and generate issue reports.

[6pac]

Hmmm, OK I'll have a think. I hadn't considered these options.

That said, creating a POCO with a PK without having retrieved it from the DB, or taken care to clear the table first, is surely an anti-pattern?
Don't want to be rude, but I hadn't considered these to be breaking changes, just fixing of behaviour that was not taking all use cases into account. I just can't think of a good reason why it should behave as it does!
I am kind of forced to regard the save() method as broken as well if it doesn't deal with that use case.

But anyway, will consider the consequences.

[asherber]

Isn't "creating a POCO with a PK without having retrieved it from the DB" exactly the use case you're trying to solve for? I thought I understood that you were creating a new object with a PK value; Update doesn't work because the record doesn't exist, and Insert doesn't work because PP doesn't send the PK value.

I think that a change is breaking if it alters established and non-buggy behavior. The current behavior of IsNew(), Save() etc. is by design and well established. Even though it's suboptimal, there are likely users whose code depends on that behavior.

[6pac]

My apologies for barging in here rather suddenly. I am really busy at the moment, but the code I'm working on will cover this issue from every angle. I'll make what changes are necessary on my private repo and get back here with something more coherent later on. thanks ...

[pleb]

The methods Save and IsNew are somewhat dumb. PP is lightweight and complexities, such as this, can be hard to solve easily. Save and IsNew, I think from memory, will only work when the DB assigns the values for the PK.

The protected method protected virtual bool IsNew(string primaryKeyName, PocoData pd, object poco) is virtual, so you could extend and implement your own logic.

🤔 There is maybe an opportunity to allow hooking this via the fluent configuration....