jraddaoui/bump_term_chat.txt

## bump_term_chat.txt
mcantelon [21:21]
@djuhasz The bumpTerm function, in migrations, will take over the ID of another term if that term's using its ID, but it doesn't work if the ID's used by some other object tyle like an information object. Seems like we can't really safely have IDs be constants unless I'm missing something.

djuhasz [22:47]
@mcantelon hmm, I haven't touched static object IDs for many many years and I've never used the bumpTerm function.  Maybe @radda @sbreker or @sevein can provide some guidance for you?

mcantelon [22:49]
@djuhasz Thanks! Yeah, I'm thinking I'm probably going to have to just buckle down and look up the term ID using the name (then caching the ID).

sevein [22:49]
@mcantelon sorry can't remember :(

mcantelon [22:50]
@sevein No worries!

sbreker [23:28]
Hi @mcantelon- I can have a look later at this....let me know if you figure it out in the meantime

mcantelon [23:30]
@sbreker Thanks Steve... I'm probably going to end up just looking up the term ID by name as I suspect there's no way to bump most object types.

mcantelon [19:13]
@radda I ran into an issue with a migration... just running this past you in case you know anything about it.... the bumpTerm function (and all the other bump* functions probably), in migrations, will take over the ID of another term if that term's using its ID, but it doesn't work if the ID's used by some other object type like an information object. Seems like we can't really safely have IDs be constants unless I'm missing something.
@radda I can work around via doing a lookup by name and caching the ID, but just wanted to see if you have any insight.

radda [19:13]
:eyes:

djuhasz [19:20]
@radda I thought we had existing code for caching a term name -> id lookup table somewhere.
@radda I'm not having any luck finding it though
There's this for settings: https://github.com/artefactual/atom/blob/qa/2.5.x/lib/filter/QubitSettingsFilter.class.php

radda [19:22]
we have it for the settings, and something similar in Binder for some terms
but not in AtoM
so, I'm looking to the bump functions ...
https://github.com/artefactual/atom/blob/qa%2F2.5.x/lib/task/migrate/QubitMigrate.class.php#L474
would it work if we check what object type will be updated, based on the id, before changing the foreign keys?
to set that to `QubitInformationObject::TABLE_NAME` instead of `QubitTerm::TABLE_NAME` if that's the case
maybe create a bumpObject function to do so
change https://github.com/artefactual/atom/blob/qa%2F2.5.x/lib/task/migrate/QubitMigrate.class.php#L482-L485 to use `QubitObject` and get the class_name from the result

djuhasz [19:29]
@mcantelon what term is this affecting? Where's is it used in the code? (edited)

mcantelon [20:00]
@djuhasz The audit log's migration makes a couple of new terms (corresponding to creation and modification).
@radda I think that if we change an information object's ID there may be repercussions given everything that relates to them. (edited)
@radda An we'd have to allow for every type of QubitObject as they all share the same primary key sequence. (edited)

radda [20:06]
yeah, that's why I don't feel confident about it, but looking at the query, it should update all relations, for the QubitObject and for the subclass
I don't really like it, but I think it could work
a nested-set update (just in case) and a search-populate after (edited)
like it's suggested in the upgrade ...

mcantelon [20:08]
@radda I tried changing bumpTerm so it just changes the ID in QubitObject and it didn't work.
(It didn't cascade to anything.)
Maybe that's the wrong way to do it?

radda [20:10]
it should change both ids, right? not just the QubitObject one

mcantelon [20:11]
@radda I'll try it with that.

radda [20:14]
@mcantelon you may need to add more cases in here: https://github.com/artefactual/atom/blob/qa%2F2.5.x/lib/task/migrate/QubitMigrate.class.php#L456
I was assuming the `findForeignKeys` function to get all related columns for the type, and it looks like it does it, but to add the informationobject columns it will need another case (edited)
an considering that the same may happen for other resources this could be more work than what I expected
but it will happen again if we don't solve it, and I don't like caching ids by term name either

mcantelon [20:19]
@radda Part of the prob is I need to do the migration for a project we want deployed by the end of this week. :disappointed: So I might have to do a quick fix (name lookup) then we can work out a legit solution to this problems. (edited)
@djuhasz What's your take, monsieur? ^

radda [20:20]
yeah, that works for me, but add a huge note if it enters vanilla :smile: (edited)

mcantelon [20:21]
@radda Will do haha. Note in the commit or ?

radda [20:21]
commit, code, issue
wiki, docs
heheh
wherever you think we won't forget, but I think we won't

djuhasz [20:22]
@mcantelon I prefer looking up the settings by name to doing key juggling
And caching the lookup if necessary for performance

mcantelon [20:23]
@radda Haha gotcha. :wink:

radda [20:23]
it could be moved to a setting?

djuhasz [20:23]
sorry, I meant "Term" not "setting"

radda [20:23]
that's the big deal, that terms are user editable

mcantelon [20:23]
I could store the IDs of those terms in a setting and cache them that way. Whatever caching method is easiest I guess.
@radda I guess we can't lock terms like we can menu items, eh? Darn. :<

djuhasz [20:24]
@mcantelon how often do you need to lookup the terms? I wonder if caching is even necessary?

radda [20:24]
I mean if a user creates a term with the same name, for example a place

mcantelon [20:24]
@djuhasz Whenever an information object is saved and audit logging is turned on.

djuhasz [20:25]
oh yeah, that sounds like it needs caching :slightly_smiling_face:

mcantelon [20:25]
@radda This is in it's own taxonomy so it would be a taxonomy plus name lookup.
@radda So it's probably fine as far as user tampering, but still something to consider (users will break anything given enough time :wink: ).

radda [20:26]
I guess if it's in a locked taxonomy and it's a protected term it will reduce the possibility of mismatch
it's not the best protection system, as taxonomy URLs are accessible even if they're locked
(we may have fixed that one at some point)

mcantelon [20:27]
@radda Cool... I'll make a note to look at that too (HTTP forbidden if locked).

radda [20:28]
awesome @mcantelon!

mcantelon [20:29]
@radda Thanks Radda! Migrations are so fun gahhhhh. ;D

sbreker [22:24]
Hey @mcantelon I figured out the term thing.

mcantelon [22:25]
@sbreker Ah cool... what's the prognosis?

sbreker [22:25]
The term constants in QubitTerm.php need to be consecutive for the term bump to work
you left a gap and jumped up to 4nn something
so the constants get added first and then the rest of the terms that get auto assigned an id. If you add a new term constant, and give it the next available id in QubitTerm.class.php, on upgrade, the id you picked should be a QubitTerm with an auto assigned id.
I tried setting your constants to 189 and 190, and they caused the terms they overlapped with to get bumped correctly when the migration was run.

mcantelon [22:37]
Interesting. I'll try that out.
@sbreker ^

sbreker [22:38]
I am scrolling up and just realized you've been talking about this all morning...
I made this change and the migration will run correctly with the 2.4.1 database that I used to trigger it initially
https://github.com/artefactual/atom/compare/dev/issue-99999-terms-bump-issue

sbreker [22:53]
@mcantelon I think there _is_ a bug here though. The bump logic relies on the non-constant terms directly following the constant ID's in the database id sequence. When the non-constant terms get bumped, they get bumped to the next available object.id.

mcantelon [22:53]
@sbreker Will the migration work if another type of object, like an information object, has the target ID before the bumpTerm?

sbreker [22:55]
no it will not as far as I know.  But that should not happen if you pick the next contiguous term constant ID.
because when atom's schema is first created and loaded from the yml file, the constant terms are created first, then all terms that are not constants are created.  The next available ID _should_ always be a term. (edited)
...that is, unless the all the dynamically assigned terms were already bumped!

mcantelon [22:59]
@sbreker I mean if you're running tools:sql-upgrade on an existing install, rather than a fresh install...

sbreker [23:01]
yes - exactly
trying to come up with an example....
sorry - have meeting now I forgot about!
will be back.

mcantelon [23:09]
@sbreker No worries... I'll try swapping the IDs and testing an upgrade with an information object taking up one of the target IDs and see what happens.

nick [23:20]
@kelly @sbreker — for the TAMU estimates, I created this proposal ticket in Redmine (https://projects.artefactual.com/issues/12416). By my count we’ve agreed to do 4 estimates, but let me know if it looks like anything’s missing. also, I don’t think we should start doing these estimates until they get their Archon export data to us

kelly [23:20]
don’t bug us nick we’re working
on TAMU

nick [23:20]
:disappointed:

kelly [23:20]
:hugging_face:

mcantelon [23:25]
@sbreker Ah, I think I get what you're saying. Because a bunch of child terms get created there's a certain range, within the object table, of safe to use term IDs . (edited)
@sbreker I was avoided using the IDs of these child terms, but that was exactly what I should have been doing hahahah. :open_mouth:

mcantelon [23:46]
@sbreker Yeah, that works like a charm.
@sbreker Thanks!!!

sbreker [23:49]
@mcantelon Yeah, it was a puzzler!  But yes you got it: the terms that get created that are not constants, are guaranteed to be what gets bumped, as long as you choose the next sequential ID.

mcantelon [23:50]
@sbreker I should have deduced that given all the other ones are sequential... d'oh. ;D

sbreker [23:51]
I can see an edge case where, if an atom instance was not upgraded for a really long time, and there were a lot of new constant terms being added from 2.0.0 to say 2.35.1 in one upgrade for instance, that you could maybe run out of terms to bump?  :thinking_face:
Unlikely...
:smile:

mcantelon [23:53]
@sbreker Very true, alas. Hopefully it won't bite anyone. :<

radda [17:18]
hi @sbreker and @mcantelon, I added a note in the PR ...
https://github.com/artefactual/atom/pull/777
mcantelon
#777 Fixed term ID issues with migration, refs #12083
Fixed an issue with the migration that adds two terms used by the audit log
functionality.
artefactual/atomToday at 00:04Added by GitHub
it's basically what you're saying in here @sbreker
and the more terms with fixed IDs we add, the higher is the possibility of finding the issue
@djuhasz ^^

sbreker [17:33]
Hi @radda - I saw your comment - yes, makes sense. Unsure of what the priority of such a fix should be vs the likelihood of it happening, or what the complexity of the fix is.

radda [19:42]
@sbreker, I shared some ideas above, they will need some tweaks and a lot of tests, but I think it could be done
looking at the stable/2.0.x branch, it looks like we have around 200 terms ...

https://github.com/artefactual/atom/blob/stable%2F2.0.x/data/fixtures/taxonomyTerms.yml
so we're close
and it will be worst for old ICA-AtoM instances upgraded over the time

sbreker [19:45]
Yes... so maybe sooner rather than later.
Did anyone create a ticket?  Your thoughts above should be added there so we don't lose it.
Maybe AtoM3 will be ready before this is needed!  I can only hope!  :smile:

radda [19:46]
haahh, that'd be awesomeeee
I'll create the ticket and paste this conversation, thanks @sbreker!

sbreker [19:47]
@radda np!
@radda Thank you too!
	mcantelon [21:21]
	@djuhasz The bumpTerm function, in migrations, will take over the ID of another term if that term's using its ID, but it doesn't work if the ID's used by some other object tyle like an information object. Seems like we can't really safely have IDs be constants unless I'm missing something.

	djuhasz [22:47]
	@mcantelon hmm, I haven't touched static object IDs for many many years and I've never used the bumpTerm function. Maybe @radda @sbreker or @sevein can provide some guidance for you?

	mcantelon [22:49]
	@djuhasz Thanks! Yeah, I'm thinking I'm probably going to have to just buckle down and look up the term ID using the name (then caching the ID).

	sevein [22:49]
	@mcantelon sorry can't remember :(

	mcantelon [22:50]
	@sevein No worries!

	sbreker [23:28]
	Hi @mcantelon- I can have a look later at this....let me know if you figure it out in the meantime

	mcantelon [23:30]
	@sbreker Thanks Steve... I'm probably going to end up just looking up the term ID by name as I suspect there's no way to bump most object types.

	mcantelon [19:13]
	@radda I ran into an issue with a migration... just running this past you in case you know anything about it.... the bumpTerm function (and all the other bump* functions probably), in migrations, will take over the ID of another term if that term's using its ID, but it doesn't work if the ID's used by some other object type like an information object. Seems like we can't really safely have IDs be constants unless I'm missing something.
	@radda I can work around via doing a lookup by name and caching the ID, but just wanted to see if you have any insight.

	radda [19:13]
	:eyes:

	djuhasz [19:20]
	@radda I thought we had existing code for caching a term name -> id lookup table somewhere.
	@radda I'm not having any luck finding it though
	There's this for settings: https://github.com/artefactual/atom/blob/qa/2.5.x/lib/filter/QubitSettingsFilter.class.php

	radda [19:22]
	we have it for the settings, and something similar in Binder for some terms
	but not in AtoM
	so, I'm looking to the bump functions ...
	https://github.com/artefactual/atom/blob/qa%2F2.5.x/lib/task/migrate/QubitMigrate.class.php#L474
	would it work if we check what object type will be updated, based on the id, before changing the foreign keys?
	to set that to `QubitInformationObject::TABLE_NAME` instead of `QubitTerm::TABLE_NAME` if that's the case
	maybe create a bumpObject function to do so
	change https://github.com/artefactual/atom/blob/qa%2F2.5.x/lib/task/migrate/QubitMigrate.class.php#L482-L485 to use `QubitObject` and get the class_name from the result

	djuhasz [19:29]
	@mcantelon what term is this affecting? Where's is it used in the code? (edited)

	mcantelon [20:00]
	@djuhasz The audit log's migration makes a couple of new terms (corresponding to creation and modification).
	@radda I think that if we change an information object's ID there may be repercussions given everything that relates to them. (edited)
	@radda An we'd have to allow for every type of QubitObject as they all share the same primary key sequence. (edited)

	radda [20:06]
	yeah, that's why I don't feel confident about it, but looking at the query, it should update all relations, for the QubitObject and for the subclass
	I don't really like it, but I think it could work
	a nested-set update (just in case) and a search-populate after (edited)
	like it's suggested in the upgrade ...

	mcantelon [20:08]
	@radda I tried changing bumpTerm so it just changes the ID in QubitObject and it didn't work.
	(It didn't cascade to anything.)
	Maybe that's the wrong way to do it?

	radda [20:10]
	it should change both ids, right? not just the QubitObject one

	mcantelon [20:11]
	@radda I'll try it with that.

	radda [20:14]
	@mcantelon you may need to add more cases in here: https://github.com/artefactual/atom/blob/qa%2F2.5.x/lib/task/migrate/QubitMigrate.class.php#L456
	I was assuming the `findForeignKeys` function to get all related columns for the type, and it looks like it does it, but to add the informationobject columns it will need another case (edited)
	an considering that the same may happen for other resources this could be more work than what I expected
	but it will happen again if we don't solve it, and I don't like caching ids by term name either

	mcantelon [20:19]
	@radda Part of the prob is I need to do the migration for a project we want deployed by the end of this week. :disappointed: So I might have to do a quick fix (name lookup) then we can work out a legit solution to this problems. (edited)
	@djuhasz What's your take, monsieur? ^

	radda [20:20]
	yeah, that works for me, but add a huge note if it enters vanilla :smile: (edited)

	mcantelon [20:21]
	@radda Will do haha. Note in the commit or ?

	radda [20:21]
	commit, code, issue
	wiki, docs
	heheh
	wherever you think we won't forget, but I think we won't

	djuhasz [20:22]
	@mcantelon I prefer looking up the settings by name to doing key juggling
	And caching the lookup if necessary for performance

	mcantelon [20:23]
	@radda Haha gotcha. :wink:

	radda [20:23]
	it could be moved to a setting?

	djuhasz [20:23]
	sorry, I meant "Term" not "setting"

	radda [20:23]
	that's the big deal, that terms are user editable

	mcantelon [20:23]
	I could store the IDs of those terms in a setting and cache them that way. Whatever caching method is easiest I guess.
	@radda I guess we can't lock terms like we can menu items, eh? Darn. :<

	djuhasz [20:24]
	@mcantelon how often do you need to lookup the terms? I wonder if caching is even necessary?

	radda [20:24]
	I mean if a user creates a term with the same name, for example a place

	mcantelon [20:24]
	@djuhasz Whenever an information object is saved and audit logging is turned on.

	djuhasz [20:25]
	oh yeah, that sounds like it needs caching :slightly_smiling_face:

	mcantelon [20:25]
	@radda This is in it's own taxonomy so it would be a taxonomy plus name lookup.
	@radda So it's probably fine as far as user tampering, but still something to consider (users will break anything given enough time :wink: ).

	radda [20:26]
	I guess if it's in a locked taxonomy and it's a protected term it will reduce the possibility of mismatch
	it's not the best protection system, as taxonomy URLs are accessible even if they're locked
	(we may have fixed that one at some point)

	mcantelon [20:27]
	@radda Cool... I'll make a note to look at that too (HTTP forbidden if locked).

	radda [20:28]
	awesome @mcantelon!

	mcantelon [20:29]
	@radda Thanks Radda! Migrations are so fun gahhhhh. ;D

	sbreker [22:24]
	Hey @mcantelon I figured out the term thing.

	mcantelon [22:25]
	@sbreker Ah cool... what's the prognosis?

	sbreker [22:25]
	The term constants in QubitTerm.php need to be consecutive for the term bump to work
	you left a gap and jumped up to 4nn something
	so the constants get added first and then the rest of the terms that get auto assigned an id. If you add a new term constant, and give it the next available id in QubitTerm.class.php, on upgrade, the id you picked should be a QubitTerm with an auto assigned id.
	I tried setting your constants to 189 and 190, and they caused the terms they overlapped with to get bumped correctly when the migration was run.

	mcantelon [22:37]
	Interesting. I'll try that out.
	@sbreker ^

	sbreker [22:38]
	I am scrolling up and just realized you've been talking about this all morning...
	I made this change and the migration will run correctly with the 2.4.1 database that I used to trigger it initially
	https://github.com/artefactual/atom/compare/dev/issue-99999-terms-bump-issue

	sbreker [22:53]
	@mcantelon I think there _is_ a bug here though. The bump logic relies on the non-constant terms directly following the constant ID's in the database id sequence. When the non-constant terms get bumped, they get bumped to the next available object.id.

	mcantelon [22:53]
	@sbreker Will the migration work if another type of object, like an information object, has the target ID before the bumpTerm?

	sbreker [22:55]
	no it will not as far as I know. But that should not happen if you pick the next contiguous term constant ID.
	because when atom's schema is first created and loaded from the yml file, the constant terms are created first, then all terms that are not constants are created. The next available ID _should_ always be a term. (edited)
	...that is, unless the all the dynamically assigned terms were already bumped!

	mcantelon [22:59]
	@sbreker I mean if you're running tools:sql-upgrade on an existing install, rather than a fresh install...

	sbreker [23:01]
	yes - exactly
	trying to come up with an example....
	sorry - have meeting now I forgot about!
	will be back.

	mcantelon [23:09]
	@sbreker No worries... I'll try swapping the IDs and testing an upgrade with an information object taking up one of the target IDs and see what happens.

	nick [23:20]
	@kelly @sbreker — for the TAMU estimates, I created this proposal ticket in Redmine (https://projects.artefactual.com/issues/12416). By my count we’ve agreed to do 4 estimates, but let me know if it looks like anything’s missing. also, I don’t think we should start doing these estimates until they get their Archon export data to us

	kelly [23:20]
	don’t bug us nick we’re working
	on TAMU

	nick [23:20]
	:disappointed:

	kelly [23:20]
	:hugging_face:

	mcantelon [23:25]
	@sbreker Ah, I think I get what you're saying. Because a bunch of child terms get created there's a certain range, within the object table, of safe to use term IDs . (edited)
	@sbreker I was avoided using the IDs of these child terms, but that was exactly what I should have been doing hahahah. :open_mouth:

	mcantelon [23:46]
	@sbreker Yeah, that works like a charm.
	@sbreker Thanks!!!

	sbreker [23:49]
	@mcantelon Yeah, it was a puzzler! But yes you got it: the terms that get created that are not constants, are guaranteed to be what gets bumped, as long as you choose the next sequential ID.

	mcantelon [23:50]
	@sbreker I should have deduced that given all the other ones are sequential... d'oh. ;D

	sbreker [23:51]
	I can see an edge case where, if an atom instance was not upgraded for a really long time, and there were a lot of new constant terms being added from 2.0.0 to say 2.35.1 in one upgrade for instance, that you could maybe run out of terms to bump? :thinking_face:
	Unlikely...
	:smile:

	mcantelon [23:53]
	@sbreker Very true, alas. Hopefully it won't bite anyone. :<

	radda [17:18]
	hi @sbreker and @mcantelon, I added a note in the PR ...
	https://github.com/artefactual/atom/pull/777
	mcantelon
	#777 Fixed term ID issues with migration, refs #12083
	Fixed an issue with the migration that adds two terms used by the audit log
	functionality.
	artefactual/atomToday at 00:04Added by GitHub
	it's basically what you're saying in here @sbreker
	and the more terms with fixed IDs we add, the higher is the possibility of finding the issue
	@djuhasz ^^

	sbreker [17:33]
	Hi @radda - I saw your comment - yes, makes sense. Unsure of what the priority of such a fix should be vs the likelihood of it happening, or what the complexity of the fix is.

	radda [19:42]
	@sbreker, I shared some ideas above, they will need some tweaks and a lot of tests, but I think it could be done
	looking at the stable/2.0.x branch, it looks like we have around 200 terms ...

	https://github.com/artefactual/atom/blob/stable%2F2.0.x/data/fixtures/taxonomyTerms.yml
	so we're close
	and it will be worst for old ICA-AtoM instances upgraded over the time

	sbreker [19:45]
	Yes... so maybe sooner rather than later.
	Did anyone create a ticket? Your thoughts above should be added there so we don't lose it.
	Maybe AtoM3 will be ready before this is needed! I can only hope! :smile:

	radda [19:46]
	haahh, that'd be awesomeeee
	I'll create the ticket and paste this conversation, thanks @sbreker!

	sbreker [19:47]
	@radda np!
	@radda Thank you too!