Add safeguards for all queryOwnerInterface calls
ClosedPublic
Actions

Authored by bb on Aug 26 2020, 1:21 PM.

Details

Reviewers

Freagarach

Commits

rP24050: Add safeguards for all queryOwnerInterface calls

Summary

While reviewing D2810, I noticed that many of the Interfaces queried with queryOwnerInterface don't have safeguards for nonexistance. Usually not a problem since they exists, however they need to be checked.

Test Plan

grep and see all have safeguards now
Agree with the proposed safeguards
Observe effectively nothing changes

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bb created this revision.Aug 26 2020, 1:21 PM

Owners added a subscriber: Restricted Owners Package.Aug 26 2020, 1:21 PM

Harbormaster completed remote builds in B12986: Diff 13294.Aug 26 2020, 1:29 PM

bb requested review of this revision.Aug 26 2020, 1:29 PM

Seems good overall

binaries/data/mods/public/simulation/components/ProductionQueue.js
452 ↗	(On Diff #13294)	I feel like this should be a straight warning/error. Seems buggy, particularly since you've already subtracted resources.
831 ↗	(On Diff #13294)	Likewise -> makes no sense to have a tech template if the owner has no technology manager
894 ↗	(On Diff #13294)	samesies

Freagarach added a subscriber: Freagarach.Aug 27 2020, 1:38 PM

Freagarach added inline comments.

binaries/data/mods/public/simulation/components/Promotion.js
88 ↗	(On Diff #13294)	Can't we use the template value when we have no player?
binaries/data/mods/public/simulation/helpers/Commands.js
302 ↗	(On Diff #13294)	Perhaps update the warnings as well?

Silier added a subscriber: Silier.Aug 27 2020, 2:14 PM

Silier added inline comments.

binaries/data/mods/public/simulation/components/Promotion.js
88 ↗	(On Diff #13294)	no player means owner is invalid so entity is mostly dead

bb updated this revision to Diff 13330.Aug 28 2020, 6:59 PM

bb marked 2 inline comments as done.

bb added inline comments.

binaries/data/mods/public/simulation/components/ProductionQueue.js
452 ↗	(On Diff #13294)	A second look showed, we already check for `cmpTechnologyManager` in `this.GetTechnologiesList`, so strictly we don't need to check this. Don't really like querying the interface twice, but out of scope
binaries/data/mods/public/simulation/components/Promotion.js
88 ↗	(On Diff #13294)	Afaik the template doesn't contain a player ID
binaries/data/mods/public/simulation/helpers/Commands.js
302 ↗	(On Diff #13294)	not much to change here, since no `cmpPlayerEntityLimits` means no limits, hence we just pass by

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3086/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1434/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/2533/display/redirect

Freagarach added inline comments.Aug 28 2020, 7:03 PM

binaries/data/mods/public/simulation/components/Promotion.js
88 ↗	(On Diff #13294)	I meant using `+template.Promotion.RequiredXp`. But Angen may be right and it may not be needed.

Harbormaster completed remote builds in B13023: Diff 13330.Aug 28 2020, 7:03 PM

bb updated this revision to Diff 13331.Aug 28 2020, 7:56 PM

Harbormaster completed remote builds in B13024: Diff 13331.Aug 28 2020, 8:02 PM

This is complete and correct (awaiting the answer).

binaries/data/mods/public/simulation/components/ProductionQueue.js
383 ↗	(On Diff #13331)	Should one still be allowed to train a restricted entity when there is no IID_EntityLimits? To me that equals having a limit of `0` so we should return here.