Fix/#2779 very slow editing of graphical chart #3239
Conversation
- Used select to lazy load points: * dataPointDetails.jsp * dataPointEdit.jsp * dataSourceEdit.jsp * eventHandlers.jsp * pointLinks.jsp * reports.jsp * scripting.jsp * viewEdit.jsp * views.jsp * editMeta.jsp * compoundEditor.jsp * settingsEditor.jsp * pointName.jsp * editHttpSender.jsp * editPachube.jsp * editPersistent.jsp * ChartComparatorComponent.java
- Reverted package-lock.json
- Corrected ChartComparatorComponent; - Created function initPointsSelect;
- Fixed MetaData Point (editMeta.jsp);
- Corrected EventDAO.getEventHandlers
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughCentralizes datapoint selection into a new PointsContext/DataPointsSelect JS architecture; many JSPs and DWR endpoints now return/consume DwrResponseI18n wrappers (response.data.*); server adds permission-aware GetDataPointsUtils and DataPointService APIs, new Jackson serializers, and DataPointBean.enabled; Chosen-based selects removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Browser/UI
participant JS as DataPointsSelect / PointsContext
participant DWR as DWR endpoint
participant Service as DataPointService / GetDataPointsUtils
Browser->>JS: Initialize picker/editor
JS->>DWR: AJAX request (e.g., getScript/getViewComponentResponse/getPointResponse)
DWR->>Service: Request points for user & context
Service->>Service: Gather & filter points (permission-aware)
Service-->>DWR: return response.data { pointList, contextPoints, ... }
DWR-->>JS: DwrResponseI18n (response.data)
JS-->>Browser: Populate DataPointsSelect, enable/refresh UI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Corrected EventDAO.getEventHandlers
#3237 Fixed Event Handlers:
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Fix all issues with AI Agents 🤖
In @src/com/serotonin/mango/web/mvc/controller/DataPointDetailsController.java:
- Line 165: The call to ControllerUtils.addPointListDataToModel(user, point,
model) can throw a NullPointerException when point is null; either move that
call inside the existing if (point != null) block that surrounds lines up to 159
so it only runs when point is non-null, or update
ControllerUtils.addPointListDataToModel to accept a null point and handle it
safely (e.g., avoid calling point.getExtendedName() and use the id or null-safe
behavior). Choose one approach and apply it consistently: if you choose the
guard, ensure all usages of point within that block are protected; if you choose
the method change, update its signature/implementation and all call sites to
tolerate a null point.
In @src/com/serotonin/mango/web/mvc/controller/DataSourceEditController.java:
- Line 67: The code in DataSourceEditController incorrectly calls
dataPointService.generateUniqueXid() to set the DataSource XID; replace that
call so the DataSource XID is created by DataSourceService instead. Locate the
line where dataSourceVO.setXid(...) is called and change the invocation to use
DataSourceService.generateUniqueXid() (i.e., instantiate or obtain a
DataSourceService and call its generateUniqueXid method) rather than using
dataPointService, ensuring the XID uses DataSourceVO.XID_PREFIX and is checked
against the dataSources table.
In @src/org/scada_lts/dao/DataPointDAO.java:
- Around line 545-564: In getDataPointsWithLimit in DataPointDAO the SQL
fragment appended when excludeIds is non-empty starts with " AND" but
DATA_POINT_SELECT has no WHERE, producing invalid SQL; change the logic to
prepend either " WHERE" or " AND" depending on whether DATA_POINT_SELECT (or the
current templateSelectWhereId) already contains a WHERE clause (e.g., check
templateSelectWhereId.toString().contains(" WHERE ") or maintain a boolean
hasWhere), then append the NOT IN(...) predicate using the correct separator and
keep adding args as done now.
In @src/org/scada_lts/mango/adapter/MangoDataPoint.java:
- Around line 100-114: Add JavaDoc comments to the eight new interface methods
and their implementations: getDataPoints(Comparator<DataPointVO> comparator,
boolean includeRelationalData, Set<Integer> excludeIds, int offset, int limit),
getDataPoints(Set<Integer> ids), getDataPoints(String searchText, Set<Integer>
excludeIds, int offset, int limit), getDataPointsWithAccess(User user, boolean
includeRelationalData), getDataPointIdWithAccessPrev(User user, String
dataPointName), getDataPointIdWithAccessNext(User user, String dataPointName),
getDataPointIdentifiersWithAccess(User user), and getDataPoints(Set<Integer>
pointIds, User user); for each JavaDoc include a one-line description of
purpose, @param tags for each parameter, and an @return describing the returned
List or id (or null/negative when not found), matching the style of the existing
getDataPointsWithAccess(User) docs and apply these comments in both implementing
classes (DataPointService and OnlyMigrationDataPointService).
In @src/org/scada_lts/utils/GetDataPointsUtils.java:
- Around line 139-146: In getDataPointsByChildren replace the direct
Integer.parseInt call on childPointIds.stream() with safe validation: parse each
pair.getValue() defensively (e.g., use a regex/NumberUtils or a try-catch around
Integer.parseInt) and skip or log entries that fail parsing so a
NumberFormatException cannot propagate; then collect only the successfully
parsed ints into the ids Set used by dataPointService.getDataPoints, preserving
the existing permission filter
(GetDataPointsWithAccess.hasDataPointReadPermission) and mapping (converter).
In @src/org/scada_lts/web/mvc/api/DataPointApiService.java:
- Around line 277-301: The current validation call in searchDataPoint
(checkArgsIfEmptyThenBadRequest) forces keywordSearch to be non-empty which
blocks the intended "search by includeIds only" flow; update the validation to
require at least one of keywordSearch or includeIds (e.g., call
checkArgsIfEmptyThenBadRequest with both searchDataPointJson.getKeywordSearch()
and searchDataPointJson.getIncludeIds(), or replace it with a conditional that
throws BadRequest unless keywordSearch is non-empty or includeIds is
non-null/non-empty) so the later logic that calls getDataPoint(includeIds) and
filteredDataPoints/filterByDataTypes still works; also change the error message
to mention "keywordSearch or includeIds cannot be empty" to reflect the new
semantics.
In @webapp-resources/messages_en.properties:
- Line 3440: The UI message for the key chosen.selector.selectPoint contains a
grammatical error ("Enter name point..."); update the value for
chosen.selector.selectPoint to a clearer phrase such as "Enter point name...",
"Type point name...", or "Search by point name..." (pick the variant that best
fits the input behavior) and ensure punctuation/ellipsis matches other messages
for consistency.
In @webapp-resources/messages_fi.properties:
- Line 3565: The Finnish resource contains English text for the key
chosen.selector.selectPoint; update the value for chosen.selector.selectPoint in
messages_fi.properties to a proper Finnish translation (e.g., "Anna pisteelle
nimi...") so the Finnish UI shows localized text and preserve the trailing
ellipsis/formatting used by the chooser.
In @webapp-resources/messages_fr.properties:
- Line 3434: The properties file contains English text for the key
chosen.selector.selectPoint; replace the incorrect English "Enter name point..."
with a proper French translation. Update the value for
chosen.selector.selectPoint to a correct French string such as "Saisir le nom du
point..." (or "Entrez le nom du point..." if imperative/formal tone is
preferred) so the French resource file contains proper localized text.
- Line 3458: The property publisherEdit.point.xid currently has English/acronym
value "XID" in the French messages file; confirm whether this key should remain
an acronym or be localized and then replace the value with the appropriate
French text (or keep "XID" if that's the intended technical acronym). Update the
publisherEdit.point.xid entry in webapp-resources/messages_fr.properties to the
confirmed French string so the French locale contains the correct translation.
In @webapp-resources/messages_lu.properties:
- Line 3453: The message key chosen.selector.selectPoint contains awkward
English "Enter name point..."; update its value to a grammatically correct
phrase such as "Enter a point name..." (or "Enter point name..." if brevity
preferred) so the UI reads naturally.
In @webapp-resources/messages_nl.properties:
- Line 3555: The Dutch localization has English text for the key
chosen.selector.selectPoint; replace the RHS string with a proper Dutch
translation (e.g., "Voer de naam van het punt in...") so the
chosen.selector.selectPoint property is fully localized and consistent with the
rest of messages_nl.properties.
In @webapp-resources/messages_pl.properties:
- Line 3577: The translation value for the key chosen.selector.selectPoint is
grammatically incorrect; update its text to a clearer phrasing such as "Enter
point name..." or "Enter the name of the point..." (replace the current "Enter
name point...") so the message reads naturally.
In @webapp-resources/messages_pt.properties:
- Line 3592: The property chosen.selector.selectPoint contains English text;
replace the value "Enter name point..." with a Portuguese translation such as
"Insira o nome do ponto..." so the entry reads
chosen.selector.selectPoint=Insira o nome do ponto... (ensure ellipsis/spacing
matches project style).
In @webapp-resources/messages_ru.properties:
- Line 3588: The property chosen.selector.selectPoint is still in English;
replace its value with a natural Russian translation (e.g., "Введите название
точки..." or another approved Russian copy) in the messages_ru.properties file
so the placeholder is localized; update the line for chosen.selector.selectPoint
accordingly and verify encoding/escaping if necessary.
In @WebContent/resources/common.js:
- Around line 1566-1576: The current writeContextArray implementations (e.g.,
ScriptPointsContext.writeContextArray and similar methods in
ReportPointsContext, SenderPointsContext) concatenate unescaped values like
data.pointName, data.pointXid, data.pointType and data.scriptVarName directly
into HTML, creating XSS risk; update each writeContextArray to escape all
user-controlled strings by calling the existing escapeHtml helper (defined
earlier in the file) when inserting into spans, attributes, or input values, and
ensure values interpolated into attribute contexts (like input value and
onclick) are properly escaped or encoded to prevent injection.
- Around line 2179-2181: The code in initPointsSelect redundantly constructs two
DataPointsSelect instances; create only one DataPointsSelect (e.g., let
dataPointsSelect = new DataPointsSelect(ref)), call dataPointsSelect.clear(),
and then return that same dataPointsSelect instead of allocating a second new
DataPointsSelect; references: DataPointsSelect and the local variable
dataPointsSelect.
In @WebContent/WEB-INF/jsp/dataPointDetails.jsp:
- Line 174: The <select> element with id "datPointDetailsPointSelect" is using a
self-closing tag which is invalid; replace the self-closing form with a proper
opening tag and a matching closing </select> (i.e., <select
id="datPointDetailsPointSelect"
onchange="window.location='data_point_details.shtm?dpid='+ this.value;"> ...
</select>) and ensure the existing option elements (and any logic that sets the
selected option/value) remain between the tags so the value binding and onchange
continue to work.
In @WebContent/WEB-INF/jsp/dataSourceEdit/editMeta.jsp:
- Line 137: The onclick on the <tag:img> passes this.value (undefined because
this is the image); update the handler to read the value from the actual select
element and pass that to pointsContext.addPointToContext. For example, give the
related <select> an id (or use an existing id/name) and call
pointsContext.addPointToContext(document.getElementById('yourSelectId').value)
from the image onclick, or if the select is adjacent use
this.previousElementSibling.value after ensuring the DOM relationship; ensure
the select has the referenced id/position so the handler reads the correct
value.
- Around line 79-80: The code uses this.pointsContext inside savePointImpl (and
again at the other occurrence) but pointsContext is a global variable declared
separately, so using this may fail depending on call context; change references
from this.pointsContext to the global pointsContext (e.g., in function
savePointImpl where locator.context = this.pointsContext.convertToSave(), and
the other occurrence around line 94) so the global pointsContext.convertToSave()
is invoked directly.
In @WebContent/WEB-INF/jsp/include/compoundEditor.jsp:
- Line 213: Remove the debug console.log statements left in the editor code
(e.g. console.log('this.component: ', this.component) and the other console.log
calls at the reported locations); either delete them or replace with the app's
proper logging mechanism (e.g. a debug logger or a conditional log gated by an
environment flag) so no raw console.debug output remains in production code;
update the surrounding functions where these calls occur to use the chosen
logger or remove the statements entirely.
- Line 210: The ternary expression using pointId currently compares against the
string literal 'undefined' which is incorrect; update the check in the
expression (the value: ... ternary that references pointId) to test for the
actual undefined value (e.g., use typeof pointId !== 'undefined' or pointId !==
undefined) and prefer strict inequality, so the expression becomes: value:
pointId && typeof pointId !== 'undefined' ? pointId : 0 (or equivalent using
pointId !== undefined).
In @WebContent/WEB-INF/jsp/pointLinks.jsp:
- Around line 27-36: The showPointLink call is passing swapped point lists to
initPointsSelect causing source selects to show target points and vice versa;
change the arguments so initPointsSelect("sourcePointId", ...) receives
response.data.sourcePoints and the selected option filtered from
response.data.sourcePoints (matching pl.sourcePointId), and
initPointsSelect("targetPointId", ...) receives response.data.targetPoints with
the selected option filtered from response.data.targetPoints (matching
pl.targetPointId). Also remove or correct the stray console.log('pl: ', data.pl)
in updatePointLink (log the actual pl or delete), and reconcile the global vars
sourcePointId/targetPointId with the let declarations in savePointLink (either
remove the unused globals or reuse them) to eliminate shadowing and unused
variables.
In @WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jsp:
- Around line 24-25: Remove the unused variable declaration allPoints from the
top-level script; locate the lines declaring "var allPoints = new Array();" and
delete that statement so only the active pointsContext (SenderPointsContext)
variable remains (e.g., leave "var pointsContext;") to clean up the dead code.
In @WebContent/WEB-INF/jsp/publisherEdit/editPachube.jsp:
- Line 55: The savePublisherImpl function incorrectly uses
this.pointsContext.convertToSave(); change the reference to the standalone
variable pointsContext so it calls pointsContext.convertToSave() instead, and
ensure any other uses of this.pointsContext inside savePublisherImpl are changed
to pointsContext so the function uses the local variable rather than relying on
this binding.
In @WebContent/WEB-INF/jsp/publisherEdit/editPersistent.jsp:
- Line 55: The call uses this.pointsContext.convertToSave() but pointsContext is
defined as a top-level/global variable (pointsContext) rather than a property on
the current object; replace this.pointsContext with the global pointsContext
(i.e., call pointsContext.convertToSave()) or alternatively ensure pointsContext
is assigned as a property on the relevant object before use; update the
reference in the function containing the line with pointsContext/convertToSave
to consistently use the same symbol (pointsContext) to avoid undefined in strict
mode or different execution contexts.
In @WebContent/WEB-INF/jsp/reports.jsp:
- Line 491: The onclick on the tag:img element incorrectly uses this.value
(undefined for an image); update the tag:img onclick so it retrieves the
associated select element's value and passes that value into
pointsContext.addPointToContext instead of this.value (e.g., locate the related
select via its id or via DOM traversal from the image like nearest parent then
querySelector for the select, and call
pointsContext.addPointToContext(select.value)); modify the onclick attribute on
the tag:img to use that select-derived value.
In @WebContent/WEB-INF/jsp/views.jsp:
- Around line 259-264: Add the missing include for
WebContent/resources/common.js before the loadPointsSelects() definition so
initPointsSelect is available (insert a script tag referencing
resources/common.js), and wrap the loop inside loadPointsSelects() with error
handling: check that initPointsSelect is a function and wrap each
initPointsSelect call in a try/catch that logs clear diagnostic information
(including the select id and caught error) to console or an appropriate logger
to aid debugging when initialization fails.
- Around line 49-50: The Chosen CSS/JS loaded by loadjscssfile is asynchronous
so loadPointsSelects() may run before the plugin exists; update loadjscssfile
usage to provide a callback or promise and only call loadPointsSelects() after
it resolves OR change loadPointsSelects()/initPointsSelect() to guard for and
wait on the plugin (e.g., check for jQuery.fn.chosen and defer/poll or attach
the script onload) so initPointsSelect() never calls .chosen() before the plugin
is available.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
webapp-resources/messages_nl.properties (1)
3556-3567: Localization inconsistency: Multiple English messages in Dutch (NL) localization file.Lines 3556-3567 contain English text that should be translated to Dutch. This affects multiple event-related keys including:
event.setPoint.*Xvariants (lines 3556-3564)event.ds.updateTimeExceededUpdatePeriodAttention(line 3565)event.ds.updateTimeExceededUpdatePeriod(line 3566)systemSettings.purgePointValuesPeriodDefault(line 3567)All these messages should be translated to Dutch to maintain consistency with the rest of this localization file.
webapp-resources/messages_fr.properties (1)
3435-3443: Add French translations for new X-suffixed event message keys.Lines 3435-3443 add nine new extended error messages with
Xsuffixes (activePointDataTypeX,activePointMissingX, etc.), but these contain only English text in the French properties file. The corresponding non-X versions are properly translated to French (e.g.,event.setPoint.activePointDataType=Type de données de point de source actif est différent que le point cible). Provide French translations for all nine new X-suffixed keys to maintain localization consistency.WebContent/WEB-INF/jsp/publisherEdit/editPachube.jsp (1)
127-139: Bug: Table colspan mismatch after adding XID column.Line 130 adds a new XID column, bringing the total column count to 7 (name, xid, status, type, feedId, dataStreamId, empty). However, line 137's
colspan="5"for the "no points" message was not updated to match the new column count.🔎 Proposed fix
- <tbody id="selectedPointsEmpty" style="display:none;"><tr><td colspan="5"><spring:message code="publisherEdit.noPoints"/></td></tr></tbody> + <tbody id="selectedPointsEmpty" style="display:none;"><tr><td colspan="7"><spring:message code="publisherEdit.noPoints"/></td></tr></tbody>src/com/serotonin/mango/web/dwr/DataSourceListDwr.java (1)
124-141: Add null check fordataPointparameter to prevent NPE.The new overload
toggleDataPoint(DataPointVO dataPoint)directly accessesdataPoint.getDataSourceId()(line 131) anddataPoint.getId()(line 138) without null validation. IfDataPointDao.getDataPoint(dataPointId)returnsnullfor an invalid/deleted point ID, this will throw aNullPointerException.🔎 Proposed fix
public DwrResponseI18n toggleDataPoint(DataPointVO dataPoint) { + if (dataPoint == null) { + DwrResponseI18n response = new DwrResponseI18n(); + response.addContextual("dataPoint", "validate.invalidValue"); + return response; + } Permissions.ensureDataSourcePermission(Common.getUser(), dataPoint.getDataSourceId());
🟡 Minor comments (16)
webapp-resources/messages_nl.properties-3555-3555 (1)
3555-3555: Localization inconsistency: English text in Dutch (NL) localization file.The message "Enter name point..." should be translated to Dutch to maintain consistency with the rest of this localization file.
🔎 Suggested Dutch translation
-chosen.selector.selectPoint=Enter name point... +chosen.selector.selectPoint=Voer puntnaam in...webapp-resources/messages_ru.properties-3588-3588 (1)
3588-3588: Localize the placeholder text for Russian usersThe value for
chosen.selector.selectPointis still in English in the Russian bundle:chosen.selector.selectPoint=Enter name point...Consider changing it to a natural Russian string, e.g.:
Proposed fix
-chosen.selector.selectPoint=Enter name point... +chosen.selector.selectPoint=Введите имя точки...webapp-resources/messages_fr.properties-3434-3434 (1)
3434-3434: Translation missing: French text expected in French properties file.Line 3434 contains English text in a French translation file. The key
chosen.selector.selectPointshould have a French translation, not English. Additionally, the English text itself is grammatically incorrect: "Enter name point..." should be "Enter point name..."Provide the correct French translation. For reference, the old value was "Select a point..." which might translate to something like "Sélectionner un point..." - the new value might be "Entrer le nom du point..." or "Saisir le nom du point..."
webapp-resources/messages_fr.properties-3458-3458 (1)
3458-3458: Translation missing: French text expected in French properties file.Line 3458 contains
publisherEdit.point.xid=XIDwith English text in a French translation file. This should have a French translation. The value "XID" should be translated to French context or kept as an acronym if that's the expected behavior, but the key's usage context suggests a French translation may be needed.Confirm the intended value and ensure it's appropriate for the French locale.
webapp-resources/messages_lu.properties-3453-3453 (1)
3453-3453: Awkward English phrasing in UI text.Line 3453: "Enter name point..." is grammatically incorrect. Consider revising to "Enter point name..." or "Enter a point name..." for clarity.
webapp-resources/messages_pt.properties-3592-3592 (1)
3592-3592: Localization issue: English text in Portuguese properties file.The message
Enter name point...should be translated to Portuguese for consistency with the rest of the file. Consider something likeInsira o nome do ponto....webapp-resources/messages_en.properties-3440-3440 (1)
3440-3440: Grammatical error in UI text.The text "Enter name point..." is grammatically incorrect. Consider one of these alternatives:
- "Enter point name..."
- "Type point name..."
- "Search by point name..."
🔎 Proposed fix
-chosen.selector.selectPoint=Enter name point... +chosen.selector.selectPoint=Enter point name...WebContent/WEB-INF/jsp/dataPointDetails.jsp-174-174 (1)
174-174: Use proper closing tag for<select>element.Self-closing
<select ... />is not valid HTML5. Some browsers may not render this correctly.🔎 Proposed fix
- <select id="datPointDetailsPointSelect" value="${point.id}" onchange="window.location='data_point_details.shtm?dpid='+ this.value;" /> + <select id="datPointDetailsPointSelect" value="${point.id}" onchange="window.location='data_point_details.shtm?dpid='+ this.value;"></select>WebContent/WEB-INF/jsp/publisherEdit/editPersistent.jsp-55-55 (1)
55-55: Inconsistent use ofthis.pointsContextvs globalpointsContext.Line 55 uses
this.pointsContextbutpointsContextis declared as a global variable at line 23, not as a property of an object. This inconsistency could lead toundefinedbeing returned ifthisrefers to the global context differently across browsers or strict mode.🔎 Proposed fix
- let points = this.pointsContext.convertToSave(); + let points = pointsContext.convertToSave();WebContent/WEB-INF/jsp/dataSourceEdit/editMeta.jsp-79-80 (1)
79-80: Inconsistent use ofthis.pointsContextfor a global variable.
pointsContextis declared as a global variable at line 25. Usingthis.pointsContextmay fail depending on the calling context. The same issue exists at line 94.🔎 Proposed fix
function savePointImpl(locator) { - locator.context = this.pointsContext.convertToSave(); + locator.context = pointsContext.convertToSave();And at line 94:
- DataSourceEditDwr.validateScript($get("script"), this.pointsContext.convertToSave(), $get("dataTypeId"), validateScriptCB); + DataSourceEditDwr.validateScript($get("script"), pointsContext.convertToSave(), $get("dataTypeId"), validateScriptCB);WebContent/WEB-INF/jsp/include/compoundEditor.jsp-210-210 (1)
210-210: String comparison with'undefined'instead of checking forundefined.The comparison
pointId != 'undefined'compares against the string literal'undefined', not the actualundefinedvalue. IfpointIdis actuallyundefined, this check won't work as intended.🔎 Proposed fix
- value: pointId && pointId != 'undefined' ? pointId : 0 + value: pointId && pointId !== undefined ? pointId : 0webapp-resources/messages_pl.properties-3577-3577 (1)
3577-3577: Verify translation text grammar.The English translation text "Enter name point..." appears grammatically incorrect. Consider revising to "Enter point name..." or "Enter the name of the point..." for better clarity.
WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jsp-24-25 (1)
24-25: Remove unusedallPointsvariable declaration.The
allPointsarray declared at line 24 is never referenced in this file. The refactoring has moved to usingpointsContext(aSenderPointsContextinstance), makingallPointsobsolete. Remove the declaration to clean up unused code.src/org/scada_lts/mango/adapter/MangoDataPoint.java-100-114 (1)
100-114: Add JavaDoc documentation to all 8 new method implementations.All implementing classes (
DataPointServiceandOnlyMigrationDataPointService) provide implementations for the 8 new interface methods. However, none of these implementations include JavaDoc documentation, while existing methods in the same class (e.g.,getDataPointsWithAccess(User)) do include JavaDoc. For consistency with the existing code style, add JavaDoc comments documenting the purpose, parameters, and return values for:
getDataPoints(Comparator, boolean, Set<Integer>, int, int)getDataPoints(Set<Integer>)getDataPoints(String, Set<Integer>, int, int)getDataPointsWithAccess(User, boolean)getDataPointIdWithAccessPrev(User, String)getDataPointIdWithAccessNext(User, String)getDataPointIdentifiersWithAccess(User)getDataPoints(Set<Integer>, User)WebContent/resources/common.js-2179-2181 (1)
2179-2181: Redundant object creation in initPointsSelect.The function creates a
DataPointsSelect, immediately callsclear()on it, then creates a new one to return. The first instance is wasted.🔎 Proposed fix
- let dataPointsSelect = new DataPointsSelect(ref); - dataPointsSelect.clear(); return new DataPointsSelect(ref);src/org/scada_lts/utils/GetDataPointsUtils.java-139-146 (1)
139-146: Missing error handling for Integer.parseInt.If
pair.getValue()returns a non-numeric string,Integer.parseIntwill throw aNumberFormatException. Consider adding validation or error handling.🔎 Proposed fix with error handling
private static <T> List<T> getDataPointsByChildren(User user, List<KeyValuePair> childPointIds, Function<DataPointVO, T> converter, DataPointService dataPointService) { - Set<Integer> ids = childPointIds.stream().map(pair -> Integer.parseInt(pair.getValue())).collect(Collectors.toSet()); + Set<Integer> ids = childPointIds.stream() + .map(pair -> { + try { + return Integer.parseInt(pair.getValue()); + } catch (NumberFormatException e) { + return null; + } + }) + .filter(Objects::nonNull) + .collect(Collectors.toSet()); return dataPointService.getDataPoints(ids) .stream() .filter(point -> GetDataPointsWithAccess.hasDataPointReadPermission(user, point)) .map(converter) .collect(Collectors.toList()); }
src/com/serotonin/mango/web/mvc/controller/DataPointDetailsController.java
Outdated
Show resolved
Hide resolved
src/com/serotonin/mango/web/mvc/controller/DataSourceEditController.java
Outdated
Show resolved
Hide resolved
- Created method GetDataPointsUtils.getContextPoints; - Refactor ControllerUtils.addPointListDataToModel, DataSourceEditController.handleRequestInternal; - Fixed DataPointDAO.getDataPointsWithLimit; - Deprecated: DataPointService.searchDataPoints(String[] keywords); - Import Chosen lib in page.tag; - Corrected function showPointLink;
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 16
Fix all issues with AI Agents 🤖
In @src/com/serotonin/mango/web/dwr/DataSourceEditDwr.java:
- Around line 395-403: togglePoint currently calls
dataPointService.getDataPoint(dataPointId) and passes the result directly to
super.toggleDataPoint, which can NPE if getDataPoint returns null for a
stale/invalid id; add a null-guard after retrieving DataPointVO (variable
dataPoint) to check for null and return a DwrResponseI18n indicating failure or
not-found (e.g., set an error message/key) instead of calling
super.toggleDataPoint, and only call
response.addData/GetDataPointsUtils.getDataPointsByDataSource when dataPoint is
non-null so you never dereference a null DataPointVO.
In @src/org/scada_lts/dao/DataPointDAO.java:
- Around line 170-200: The access-filter constant
DATA_POINT_FILTERED_BASE_ON_USER_ID_USERS_PROFILE_ID is an OR-chain and when
concatenated with DATA_POINT_PREV/DATA_POINT_NEXT the AND binds only to the last
OR; fix by wrapping the entire access-filter expression in parentheses before
concatenation (i.e. use "(" +
DATA_POINT_FILTERED_BASE_ON_USER_ID_USERS_PROFILE_ID + ") AND " +
DATA_POINT_PREV and similarly for NEXT) so the STRCMP navigation condition
applies to the whole permission filter; apply the same parenthesized
concatenation wherever the access-filter constant is combined with navigation
conditions (e.g., the DATA_POINT_PREV_ON_USER_ID_USERS_PROFILE_ID /
DATA_POINT_NEXT_ON_USER_ID_USERS_PROFILE_ID usages and any similar blocks).
In @src/org/scada_lts/utils/GetDataPointsUtils.java:
- Around line 148-155: The getDataPointsByChildren method currently calls
Integer.parseInt(pair.getValue()) without validation which can throw
NumberFormatException; update the parsing step for KeyValuePair values (in the
stream mapping of childPointIds) to safely handle non-numeric values by
attempting to parse inside a try-catch or using a filter that validates numeric
strings first (e.g., NumberUtils.isDigits or a small isNumeric helper), skip or
log invalid entries, then collect only valid Integer ids into the Set<Integer>
ids so dataPointService.getDataPoints(...) receives only valid ids and the
method won’t throw at runtime.
In @src/org/scada_lts/web/mvc/controller/DataPointEditController.java:
- Line 122: The refactor made userPoints lazy-loaded so
ControllerUtils.addPointListDataToModel(model::addAttribute, user, dataPoint)
now only adds the current point (breaking the JSP dropdown); fix by restoring a
full accessible-point collection for the view or adding an endpoint the frontend
can call: either adjust ControllerUtils.addPointListDataToModel to also load and
add a full List<DataPoint> (e.g., userPoints) alongside the current dataPoint,
or implement a new controller method (e.g., getAccessiblePoints or
/points/accessible) that returns all configurable points and update
initPointsSelect to fetch that list via AJAX; ensure the JSP <c:forEach
items="${userPoints}"> target remains populated with all accessible points or
that initPointsSelect is wired to the new endpoint.
In @WebContent/resources/common.js:
- Around line 1942-2002: The AJAX URL in DataPointsSelect's input handler builds
query params by simple string concatenation which breaks on special characters
and arrays; update the URL construction inside the jQuery.ajax call (the block
that sets url: getAppLocation() + "api/datapoints/bean?...") to use proper
encoding/serialization (e.g., build a URLSearchParams or call encodeURIComponent
on keywordSearch and serialize excludeIds and dataTypes as repeated keys or a
JSON/stringified value) so keywordSearch, excludeIds and dataTypes are safely
encoded before appending to getAppLocation() + "api/datapoints/bean".
In @WebContent/WEB-INF/jsp/dataSourceEdit/editMeta.jsp:
- Line 130: The onclick on the <tag:img> is passing this.value (undefined) to
pointsContext.addPointToContext because the image has no value; change the
handler to obtain the actual dropdown value and pass that to
pointsContext.addPointToContext (e.g., have the handler call
pointsContext.addPointToContext(document.getElementById('<your-select-id>').value)
or locate the related select via DOM traversal from the image
(closest/previousElementSibling) and pass its .value), updating the <tag:img>
onclick accordingly.
In @WebContent/WEB-INF/jsp/eventHandlers.jsp:
- Around line 433-458: activePointSelectChanged may access targetPoint.dataType
when targetPoint is null; update the function (for both branches using
targetPointIdSelect and selectedHandlerNode.targetPointIdSelect) to null-check
targetPoint before reading .dataType or passing it into
setDataTypes/updatePointsList: if targetPoint is null/undefined, call
setDataTypes([]) and updatePointsList([]) or skip adding targetPoint to the
list, and ensure subsequent calls like setPointId(inactiveId) still occur; also
guard activePoint access similarly when building the updatePointsList so you
only include activePoint when it exists.
- Around line 275-282: The code assumes allPoints.find(...) returns objects; add
null guards around targetPoint, activePoint, and inactivePoint before using
their properties or including them in arrays: ensure targetPoint exists before
accessing targetPoint.dataType (pass a safe default like null/empty array to
initPointsSelect if missing), filter any arrays passed to initPointsSelect to
remove undefined entries (e.g., build arrays with [targetPoint,
inactivePoint].filter(Boolean)), and change the selectedHandlerNode.*
assignments to use these guarded arrays and default values so no property access
occurs on undefined.
In @WebContent/WEB-INF/jsp/include/compoundEditor.jsp:
- Around line 206-212: The code in compoundEditor.jsp is comparing pointId to
the string 'undefined' which is incorrect; update the condition around the
$get(...) result (variable pointId) to check for the actual undefined/null/falsy
value instead (e.g., use typeof pointId === "undefined" or pointId == null or
simply !pointId depending on desired semantics) when pushing into childPointIds
for each pointChildren[i].id so missing selections are converted to 0 correctly.
In @WebContent/WEB-INF/jsp/pointEdit/pointName.jsp:
- Around line 68-69: Update the select with id "allPointsList" so its onchange
navigates to the editable endpoint (use "data_point_edit.shtm?dpid=" +
this.value instead of "data_point_details.shtm") and add a fallback option
inside the select that represents the current point (use the existing JSP form
values such as form.id and form.name or form.pointName) so the control is
functional if JS fails or the list is empty; ensure the onchange handler and the
fallback option are the only changes (locate the select element and its onchange
attribute to modify).
In @WebContent/WEB-INF/jsp/pointLinks.jsp:
- Around line 153-173: Remove the leftover debug logging in the two change
handlers: delete the console.log calls inside sourcePointSelectChanged and
targetPointSelectChanged so they no longer print [targetPointId, sourcePoint,
targetPointSelect] or [sourcePointId, targetPoint, sourcePointSelect]; ensure no
other stray console.* debug statements remain in the file to avoid leaking
internals in production.
In @WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jsp:
- Line 188: The code in savePublisherImpl uses this.pointsContext but
savePublisherImpl is invoked as a standalone function, so this will be
undefined; update the function to reference the correct variable (e.g., use
pointsContext.convertToSave() instead of this.pointsContext.convertToSave()), or
modify savePublisherImpl to accept pointsContext as a parameter and use that
parameter inside the function; ensure all call sites are updated to pass the
pointsContext when choosing the parameter approach.
In @WebContent/WEB-INF/jsp/scripting.jsp:
- Around line 77-78: Remove the debug console.log in the code that prints the
dataPoint; specifically delete or comment out the console.log('dataPoint:
',dataPoint) call located after the lookup using
this.pointsContext.getContextPointById(entry.key) so only the dataPoint
assignment remains and no debug output is emitted in scripting.jsp.
- Line 173: In the saveScript callback the code incorrectly references
this.pointsContext; replace that with the local pointsContext variable (same fix
as in executeScript) so the call to the save function uses
pointsContext.convertToSave() instead of this.pointsContext.convertToSave();
update any other occurrences inside the saveScript callback that reference
this.pointsContext to use pointsContext to ensure the correct context is passed.
- Around line 39-40: In the executeScript regular function, change references
from this.pointsContext to the standalone pointsContext (i.e., call
pointsContext.convertToSave() instead of this.pointsContext.convertToSave()) and
then pass that result into convertPointsOnContext to set
payload.pointsOnContext; update any other uses in executeScript that rely on
this.pointsContext to use the pointsContext variable directly so it works in
strict mode and when executeScript is invoked as a plain function.
- Line 143: Remove the debug console.log call shown as
console.log('editingScript.objectsOnContext: ', editingScript.objectsOnContext);
— delete that line (or comment it out) from the scripting.jsp so no debug output
remains; if needed replace with a proper logging mechanism or nothing, but
ensure editingScript.objectsOnContext is not logged to the console in
production.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
WebContent/WEB-INF/jsp/include/settingsEditor.jsp (1)
83-104: Avoid clearing the point list when the component has no bound data point
open()always callssettingsEditor.pointSelectChanged(comp.dataPointId), andpointSelectChangedclears the select whendataPointId <= 0. For a new settings component with nodataPointId, this will wipe outsettingsPointList, leaving the user with no options to choose from.Consider guarding the call and/or relaxing the else branch so an unassigned component still shows the full list:
Suggested minimal change
- // Update the data in the form. + // Update the data in the form. $set("settingsPointList", comp.dataPointId); $set("settingsPointName", comp.nameOverride); $set("settingsSettable", comp.settableOverride); $set("settingsBkgdColor", comp.bkgdColorOverride); $set("settingsControls", comp.displayControls); $set("settingsPositionX", comp.x); $set("settingsPositionY", comp.y); - - settingsEditor.pointSelectChanged(comp.dataPointId); + if (comp.dataPointId > 0) { + settingsEditor.pointSelectChanged(comp.dataPointId); + } @@ - this.pointSelectChanged = function(dataPointId) { - if(dataPointId > 0) { + this.pointSelectChanged = function(dataPointId) { + if (dataPointId > 0) { var point = this.dataPointsSelect.getPoint(dataPointId); @@ - } else { - this.dataPointsSelect.setPointsArray([]); - } + }This keeps the lazy‑loading behavior while preserving usability for new/unbound components.
Also applies to: 133-159
🧹 Nitpick comments (16)
WebContent/WEB-INF/jsp/include/compoundEditor.jsp (3)
152-154: Commented-out code should be removed or enabled.The call to
setDataTypesat line 153 is commented out, but the methodsetDataTypesexists (lines 311-319). Similarly, line 270 has another commented call. If this is work-in-progress, consider adding a TODO comment explaining the intent; otherwise, remove the dead code.
311-319: Unused method - consider removing or implementing the feature.
setDataTypesis defined but never called since both invocations (lines 153 and 270) are commented out. If this is planned functionality, track it with a TODO or issue; otherwise, remove to avoid dead code.
367-371: Use strict equality andpush()for clarity.Line 371 uses loose equality (
==) which may cause unexpected matches ifpoint.idanddataPointIdhave different types (e.g., string vs number). Also, preferpush()over index-based assignment for appending to arrays.🔎 Proposed fix
for (let i = 0; i < pointChildren.length; i++) { var pointChildId = "compoundPointSelect"+ pointChildren[i].id; var dataPointId = pointChildren[i].viewComponent.dataPointId; - this.targetPointSelects[this.targetPointSelects.length] = initPointsSelect(pointChildId, "<spring:message code='chosen.selector.selectPoint'/>", "400px", [], this.pointList.filter((point) => point.id == dataPointId)); + this.targetPointSelects.push(initPointsSelect(pointChildId, "<spring:message code='chosen.selector.selectPoint'/>", "400px", [], this.pointList.filter((point) => point.id === dataPointId)));WebContent/WEB-INF/jsp/pointLinks.jsp (1)
75-88: Validation logic is defensive but consider addressing root cause.The validation checks for both
!sourcePointIdandsourcePointId == 'undefined'(string comparison). While this defensive approach prevents invalid submissions, the string comparison suggests that$get()might return the string'undefined'in some scenarios rather than the JavaScript undefined value.Consider investigating whether this edge case can be prevented upstream in the
$get()implementation or the point select component.WebContent/WEB-INF/jsp/pointEdit/pointName.jsp (1)
51-51: Variable assignment unused.The variable
datPointDetailsPointSelectis assigned but never referenced in this file. SinceinitPointsSelect()initializes the Chosen dropdown for the#allPointsListelement, the return value can be discarded if it's not needed for subsequent operations.Suggested fix
- let datPointDetailsPointSelect = initPointsSelect("allPointsList", "<spring:message code='chosen.selector.selectPoint'/>", "400px", [], [point], [], true); + initPointsSelect("allPointsList", "<spring:message code='chosen.selector.selectPoint'/>", "400px", [], [point], [], true);WebContent/WEB-INF/jsp/dataPointEdit.jsp (1)
85-89:initPointsSelectintegration looks fine; local variable is unusedWiring
init()to callinitPointsSelect("selected_base_on_existing_point_chooser", …)is consistent with the new points‑selector pattern, and the laterjQuery('#selected_base_on_existing_point_chooser').find(':selected')calls will still work against the underlying<select>.You can optionally drop the unused
selectedBaseOnExistingPointChooserlocal to reduce noise:function init() { initPointsSelect("selected_base_on_existing_point_chooser", "...message..."); }src/com/serotonin/mango/web/mvc/controller/DataSourceEditController.java (1)
52-53: Consider injecting services instead of instantiating per request.Creating new service instances on every request adds overhead. Consider using dependency injection (Spring's
@Autowiredor constructor injection) to manage service lifecycles.WebContent/WEB-INF/jsp/eventHandlers.jsp (1)
600-602:this.allPointsassignment in global function.In
setAllPoints, usingthis.allPointsin a standalone function assigns to the globalwindow.allPoints, which happens to work but is confusing. Consider using justallPoints = newAllPoints;to be explicit.🔎 Proposed fix
function setAllPoints(allPoints) { - this.allPoints = allPoints; + window.allPoints = allPoints; }Or rename the parameter to avoid shadowing:
- function setAllPoints(allPoints) { - this.allPoints = allPoints; + function setAllPoints(newAllPoints) { + allPoints = newAllPoints; }src/com/serotonin/mango/web/mvc/controller/ControllerUtils.java (2)
129-148: getContextPoints correctly extracts context from MetaPointLocatorVO.The method properly checks the locator type before casting and uses the service to fetch related data points. However, consider that duplicate points could be added if multiple MetaPointLocatorVO reference the same context points.
🔎 Consider using a Set to avoid duplicates
public static List<DataPointVO> getContextPoints(User user, List<DataPointVO> allPoints, DataPointService dataPointService, Comparator<DataPointVO> comparator) { - List<DataPointVO> contextPoints = new ArrayList<>(); + Set<DataPointVO> contextPointsSet = new LinkedHashSet<>(); for (DataPointVO dp : allPoints) { if(dp.getPointLocator() instanceof MetaPointLocatorVO) { MetaPointLocatorVO pointLocatorVO = dp.getPointLocator(); Set<Integer> ids = pointLocatorVO.getContext().stream().map(IntValuePair::getKey).collect(Collectors.toSet()); List<DataPointVO> dataPoints = dataPointService.getDataPoints(ids); for(DataPointVO dataPoint: dataPoints) { - addPointIfHasPermission(dataPoint, user, contextPoints); + if (GetDataPointsWithAccess.hasDataPointReadPermission(user, dataPoint)) { + contextPointsSet.add(dataPoint); + } } } } + List<DataPointVO> contextPoints = new ArrayList<>(contextPointsSet); if(comparator != null) { contextPoints.sort(comparator); } return contextPoints; }
99-99: DataPointService instantiated per call.Similar to DataSourceEditController, instantiating services per method call adds overhead. For utility methods, consider passing the service as a parameter (as done in
getContextPoints) for consistency.WebContent/WEB-INF/jsp/scripting.jsp (1)
167-169: Inconsistentthisusage in setPointsContext.The function assigns to
this.pointsContext, which works because it's assigning to window in browser global scope, but is inconsistent with the module-levelvar pointsContextdeclaration on line 62. Consider using direct assignment for clarity.🔎 Proposed fix
function setPointsContext(newPointsContext) { - this.pointsContext = pointsContext; + pointsContext = newPointsContext; }src/org/scada_lts/mango/service/DataPointService.java (2)
643-645: Consider reducing visibility for the unfiltered overload.The
getDataPoints(Set<Integer> pointIds)method at line 643 retrieves data points without permission checks. If this is intended only for internal service-layer use, consider reducing its visibility topackage-privateorprotectedto prevent accidental misuse from user-facing code paths. The user-aware overload at line 648 provides the permission-filtered variant.
729-738: Consider validating pagination parameters.The
offsetandlimitparameters are passed directly to the DAO without validation. While the DAO may handle invalid values, adding defensive checks (e.g.,offset >= 0,limit > 0) would improve robustness and fail fast with a clear error message.src/org/scada_lts/utils/GetDataPointsUtils.java (1)
195-211: Consider clarifying the unchecked cast and avoid side-effects in peek().Two observations:
Line 199: The unchecked cast
(List<PublishedPointVO>) publisher.getPoints()could benefit from a comment or@SuppressWarnings("unchecked")annotation explaining why the cast is safe.Line 208: Using
peek()to modify object state (setEnabled) is discouraged. Thepeek()method is primarily intended for debugging, not side effects. Consider usingmap()if modification is necessary, or move the state update to a separate step for clarity.WebContent/resources/common.js (2)
1334-1349: Clarify the random step logic and remove debug logging.Two concerns:
Line 1343-1345: The random step value (
Math.floor(Math.random() * 10)) for field sizing seems unusual. Random sizing adjustments could lead to inconsistent user experience. If this is intentional, add a comment explaining why. If not, consider using a fixed step value.Line 1370: Remove the
console.log('major: ', major);statement as it's debug code that shouldn't remain in production.🔎 Proposed fix
function sizingField(lengthLimit, target, initWidth) { if(isSupportedFieldSizing()) { return; } if(!target.style.width) { initSizeField(target, initWidth); } if(target.value.length > lengthLimit) { if(target.style.width) { - var step = Math.floor(Math.random() * 10); + var step = 8; // Fixed step for consistent sizing var temp = Number.parseInt((target.style.width + "").replace('px', '')) + step; target.style.width = temp + "px"; } } return true; }And remove the console.log at line 1370:
- console.log('major: ', major); return major >= minVersion;
2161-2179: Clarify the double instantiation pattern.Lines 2176-2178 create a
DataPointsSelectinstance, immediately callclear()on it, then create and return a new instance with the same definition. This pattern seems redundant. If the goal is to ensure a clean state, theclear()call on a newly instantiated object is unnecessary. Consider either:
- Removing the first instantiation if it serves no purpose, or
- Adding a comment explaining why this pattern is needed.
🔎 Proposed fix
function initPointsSelect(selectHtmlId, placeholderTextSingle, widthPx, excludePointsArray, pointsArray, dataTypes, invisibleEmptyOption, lastEmptyOption) { let ref = {} ref.excludePointsArray = excludePointsArray || []; ref.limit = 500; ref.selectHtmlId = selectHtmlId; ref.placeholderTextSingle = placeholderTextSingle; ref.pointsArray = pointsArray || []; ref.dataTypes = dataTypes || []; ref.altKey = "id"; ref.altValue = "name"; ref.widthPx = widthPx || "400px"; ref.invisibleEmptyOption = invisibleEmptyOption || false; ref.lastEmptyOption = lastEmptyOption || false; - let dataPointsSelect = new DataPointsSelect(ref); - dataPointsSelect.clear(); return new DataPointsSelect(ref); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
WebContent/WEB-INF/jsp/dataPointDetails.jspWebContent/WEB-INF/jsp/dataPointEdit.jspWebContent/WEB-INF/jsp/dataSourceEdit/editMeta.jspWebContent/WEB-INF/jsp/eventHandlers.jspWebContent/WEB-INF/jsp/include/compoundEditor.jspWebContent/WEB-INF/jsp/include/settingsEditor.jspWebContent/WEB-INF/jsp/pointEdit/pointName.jspWebContent/WEB-INF/jsp/pointLinks.jspWebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jspWebContent/WEB-INF/jsp/publisherEdit/editPachube.jspWebContent/WEB-INF/jsp/publisherEdit/editPersistent.jspWebContent/WEB-INF/jsp/reports.jspWebContent/WEB-INF/jsp/scripting.jspWebContent/WEB-INF/jsp/views.jspWebContent/WEB-INF/jsp/watchList.jspWebContent/WEB-INF/tags/page.tagWebContent/resources/common.jssrc/com/serotonin/mango/web/dwr/DataSourceEditDwr.javasrc/com/serotonin/mango/web/mvc/controller/ControllerUtils.javasrc/com/serotonin/mango/web/mvc/controller/DataPointDetailsController.javasrc/com/serotonin/mango/web/mvc/controller/DataSourceEditController.javasrc/org/scada_lts/dao/DataPointDAO.javasrc/org/scada_lts/mango/service/DataPointService.javasrc/org/scada_lts/utils/GetDataPointsUtils.javasrc/org/scada_lts/web/mvc/controller/DataPointEditController.java
💤 Files with no reviewable changes (1)
- WebContent/WEB-INF/jsp/watchList.jsp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/com/serotonin/mango/web/mvc/controller/DataPointDetailsController.java
- WebContent/WEB-INF/jsp/views.jsp
- WebContent/WEB-INF/jsp/dataPointDetails.jsp
🧰 Additional context used
🧬 Code graph analysis (5)
src/com/serotonin/mango/web/mvc/controller/DataSourceEditController.java (3)
src/com/serotonin/mango/Common.java (1)
Common(69-584)src/com/serotonin/mango/vo/dataSource/DataSourceVO.java (1)
DataSourceVO(91-702)src/com/serotonin/mango/web/mvc/controller/ControllerUtils.java (1)
ControllerUtils(44-180)
WebContent/resources/common.js (1)
scadalts-ui/src/utils/common.js (2)
i(74-74)getAppLocation(1-12)
src/org/scada_lts/web/mvc/controller/DataPointEditController.java (1)
src/com/serotonin/mango/web/mvc/controller/ControllerUtils.java (1)
ControllerUtils(44-180)
src/com/serotonin/mango/web/mvc/controller/ControllerUtils.java (1)
src/org/scada_lts/permissions/service/GetDataPointsWithAccess.java (1)
GetDataPointsWithAccess(18-106)
src/org/scada_lts/dao/DataPointDAO.java (2)
src/org/scada_lts/dao/ViewDAO.java (1)
Deprecated(280-288)src/org/scada_lts/dao/DAO.java (1)
DAO(41-173)
🔇 Additional comments (28)
WebContent/WEB-INF/jsp/pointLinks.jsp (5)
30-42: LGTM: Init function correctly uses new data structure.The refactored initialization properly iterates over
response.data.pointLinksand passes the fullresponse.datacontext toupdatePointLink, enabling access to source and target point collections.
47-64: LGTM: Point select initialization correctly implements exclusion logic.The
initPointsSelectcalls properly use the exclusion parameter to prevent circular or conflicting point link configurations, as clarified in the previous review discussion. The initialization and selected value setting flow is correct.
129-135: LGTM: Improved function signature makes data dependency explicit.The refactored
getPointNamefunction clearly indicates its dependency on thesourcePointscollection, improving code readability and maintainability.
27-28: LGTM: Global select control references appropriately manage UI state.The global variables for select controls enable the bidirectional change handlers to maintain consistent filtering and exclusion logic across user interactions.
221-226: LGTM: Change handlers properly wired to select elements.The
onchangeattributes correctly connect the select elements to the bidirectional filtering functions, enabling dynamic constraint updates during point selection.WebContent/WEB-INF/jsp/pointEdit/pointName.jsp (2)
26-30: LGTM: Prevents default form submission behavior.The addition of
return falseappropriately prevents the default form submission since the actual submission is handled through the callback chain ending indoSaveForm().
45-50: XSS vulnerability remains unaddressed.The point properties are still rendered directly into JavaScript strings using
<c:out>, which only escapes HTML but not JavaScript special characters like quotes or backslashes. This creates a potential XSS vulnerability as previously flagged.Likely an incorrect or invalid review comment.
WebContent/WEB-INF/tags/page.tag (1)
48-49: Chosen assets now loaded globally via base page templateCentralizing the Chosen CSS/JS here aligns with the new
initPointsSelect/DataPointsSelectusage across JSPs and avoids per‑page duplication. Ordering (jQuery → common.js → chosen.js → page scripts) should be safe for plugin initialization.Also applies to: 95-97
WebContent/WEB-INF/jsp/publisherEdit/editPersistent.jsp (1)
21-41: Persistent publisher now correctly usesPersistentPointsContextThe migration from manual point arrays to
PersistentPointsContext(withconvertToSave()and context refresh insavePersistentCB) looks consistent with the new shared points‑context infrastructure. TheselectPoint()and save flows are straightforward and avoid duplicating point‑table management logic.Also applies to: 44-69, 131-141
WebContent/WEB-INF/jsp/reports.jsp (1)
26-27: Report point selection successfully migrated toReportPointsContextThe new pattern—building a
reffromresponse.data.report/response.data.points, instantiatingReportPointsContext, and usingpointsContext.convertToSave()insaveReport/runReport—fits well with the shared points‑context design and removes the previous ad‑hoc arrays.Also, the
onclick="pointsContext.addPointToContext();"on the add icon avoids the earlierthis.valuemisuse and correctly delegates selection to the context.Also applies to: 65-91, 133-135, 303-312, 376-381, 481-485
src/com/serotonin/mango/web/mvc/controller/DataSourceEditController.java (1)
105-112: Points aggregation logic looks correct for lazy loading.The refactored approach retrieves only the necessary data points for the current data source, then derives context and analog points from this reduced set. This aligns with the PR objective of improving performance for graphical chart editing.
WebContent/WEB-INF/jsp/dataSourceEdit/editMeta.jsp (1)
53-77: Context initialization refactored to use ref object pattern.The new initialization approach using a configuration object is cleaner and more maintainable. The
editPointCBImplsignature change to acceptcontextPointsenables the lazy loading optimization.WebContent/WEB-INF/jsp/publisherEdit/editPachube.jsp (2)
24-40: Points context initialization properly handles lazy loading.The initialization clears any existing context before creating a new
PachubePointsContextwith the configuration object. This prevents stale state issues on re-initialization.
62-68: Good addition of save callback for state synchronization.The
savePachubeCBfunction properly updates the points context after a successful save, ensuring the UI stays in sync with server state.WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jsp (1)
50-64: Context initialization follows consistent pattern.The
SenderPointsContextinitialization mirrors the pattern used in other publisher editors, maintaining consistency across the codebase.src/com/serotonin/mango/web/mvc/controller/ControllerUtils.java (1)
92-108: New addPointListDataToModel handles navigation with lazy loading.The refactored method uses
DataPointServiceto get prev/next IDs without loading the full point list, aligning with the performance optimization goals. The null check forpointis properly handled.src/org/scada_lts/mango/service/DataPointService.java (5)
43-43: LGTM!The new imports are appropriately used in the expanded public API methods.
Also applies to: 47-47
204-207: LGTM!The new method correctly delegates to the access control service and follows the established pattern for permission-checked retrieval.
241-244: LGTM!The deprecation is appropriate given the new
getDataPointsmethod with keyword search that provides more flexible retrieval options.
750-761: LGTM!The navigation methods correctly distinguish between admin and regular user access paths, applying appropriate permission filtering.
720-726: Breaking behavior change in keyword search implementation.The new
getDataPoints(String keywordSearch, ...)method passes the entire search string as a single keyword to the DAO, while the deprecatedsearchDataPointsBy(String searchText)method splits the text into multiple keywords usingsplit("\\s+"). This changes the search behavior: a query like "foo bar" will now only match data points containing the literal phrase "foo bar", whereas previously it searched for data points containing both "foo" AND "bar" separately.The DAO's
getDataPointByKeywordsiterates through the keyword set and builds cumulative AND conditions, so passing a single keyword fundamentally breaks multi-keyword search functionality. This is a breaking change for any consumers relying on the previous multi-keyword search behavior.src/org/scada_lts/utils/GetDataPointsUtils.java (2)
38-40: LGTM!The utility class follows best practices with a
finalmodifier and private constructor to prevent instantiation.
90-98: LGTM!The admin-only enforcement is appropriate for retrieving event handler data points, as event handlers are typically restricted to administrative access.
WebContent/resources/common.js (5)
1376-1508: LGTM!The
PointsContextbase class provides a solid foundation for the context hierarchy with appropriate lifecycle methods and clear separation of concerns.
1510-1587: LGTM with known XSS issue to be addressed.The
ScriptPointsContextimplementation is logically sound. The XSS vulnerability inwriteContextArray(lines 1566-1576) is already documented in past review comments and acknowledged by the developer for future remediation.
1589-1679: LGTM with known XSS issue to be addressed.The
ReportPointsContextfollows the established pattern. The XSS vulnerability inwriteContextArrayis covered by the existing review comment on the base implementation pattern.
1681-1940: LGTM with known XSS issue to be addressed.The
SenderPointsContext,PersistentPointsContext, andPachubePointsContextclasses maintain consistent implementation patterns. The XSS vulnerabilities in their respectivewriteContextArraymethods are covered by the existing review comment.
2004-2084: LGTM!The getter/setter methods and
clear()method are well-implemented with appropriate defensive checks and proper cleanup.
- Encoded keywordSearch; - Removed pointer this; - Removed debug console log; - Removed forEach for select 'selected_base_on_existing_point_chooser' in dataPointEdit.jsp; - Corrected query in DataPointDAO: replacing the STRCMP function, corrected query or/and; - Added search by starts with word in DataPointDAO.getDataPointByKeywords;
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI Agents
In @src/com/serotonin/mango/rt/dataSource/meta/ScriptExecutor.java:
- Around line 214-216: In ScriptExecutor.java, replace the two LOG.warn(...)
calls that precede throwing new ScriptException(...) with LOG.error(...) so
script evaluation failures are logged at error level; include the exception
object in the log call (e.g., LOG.error("Error evaluating string (script): " +
e.getMessage(), e)) to preserve stacktrace/context and keep logging consistent
with MetaPointLocatorRT and ScriptComponent.
In @src/org/scada_lts/dao/DataPointDAO.java:
- Around line 187-191: The SQL constants DATA_POINT_NEXT and DATA_POINT_PREV use
CONCAT_WS (unsupported on Oracle/older SQL Server); update them so they do not
rely on CONCAT_WS—either rewrite the WHERE to compare ds.name and dp.name
separately (e.g. use tuple/lexicographic comparison or WHERE (ds.NAME > ? OR
(ds.NAME = ? AND dp.NAME > ?)) with corresponding ORDER BY ds.NAME ASC, dp.NAME
ASC LIMIT 1) and similarly for PREV, or implement database-specific
concatenation via branching in the DAO; ensure you update the callers
selectDataPointIdWithAccessNext() and selectDataPointIdWithAccessPrev() to pass
the additional parameters if you choose separate-parameter comparison so the
queries work on Oracle and pre-2017 SQL Server.
In @src/org/scada_lts/web/mvc/api/DataPointApiService.java:
- Around line 409-412: The predicate in filterByDataTypes currently returns true
when a DataPointVO has a null point locator, allowing such points to pass the
dataTypes filter; change the locator check so that when dataTypes is
non-null/non-empty the predicate requires a non-null pointLocator and that its
dataTypeId is contained in dataTypes. Concretely, in filterByDataTypes replace
the clause "(dataPoint.getPointLocator() == null ||
dataTypes.contains(dataPoint.getPointLocator().getDataTypeId()))" with a check
that ensures pointLocator is non-null and its getDataTypeId() is in dataTypes
(e.g. "dataPoint.getPointLocator() != null &&
dataTypes.contains(dataPoint.getPointLocator().getDataTypeId())") while keeping
the existing short-circuit for null/empty dataTypes from
searchDataPointJson.getDataTypes().
In @WebContent/resources/common.js:
- Around line 2162-2172: The helper #getId should not return -1 as a sentinel
because it can produce false matches and be sent to the API; change
#getId(object) to return null (or undefined) when no id property is found, then
update all callers such as #loadPoints and updatePointsList to filter out
null/undefined IDs before using them (e.g., remove nulls from excludeIds and
skip comparisons where #getId(...) is null) so no invalid IDs are sent or
compared.
- Around line 1376-1391: The PointsContext constructor calls the abstract init()
which currently throws, causing immediate failures; update PointsContext so it
no longer invokes an unimplemented method: either make PointsContext explicitly
abstract by removing the direct call to this.init(...) from the constructor (so
subclasses must call init themselves after super) or replace the throwing init()
with a safe default no-op implementation that accepts the context parameter and
does nothing; ensure references to DataPointsSelect (updatePointsArray) remain
initialized in the constructor and document in the class comment that subclasses
should override init(context) when custom behavior is required.
In @WebContent/WEB-INF/jsp/eventHandlers.jsp:
- Around line 610-612: The setAllPoints function assigns to this.allPoints which
can be undefined when invoked in the global context; change it to assign the
global allPoints variable directly (replace this.allPoints = allPoints with
allPoints = allPoints) or, even better, remove setAllPoints entirely and assign
the value directly inside the callback where setAllPoints is called (eliminate
function setAllPoints and set global allPoints in that callback).
- Around line 443-469: In activePointSelectChanged, add null/undefined guards
around targetPoint (from selectedHandlerNode.targetPointIdSelect.getPoint() and
targetPointIdSelect.getPoint()) before accessing targetPoint.dataType: either
return early if targetPoint is missing or call setDataTypes with an empty array
and updatePointsList appropriately; update both branches (the
selectedHandlerNode branch and the else branch) to avoid dereferencing
targetPoint when it is null/undefined.
- Around line 471-497: In inactivePointSelectChanged(), add null/undefined
guards for the values returned by targetPointIdSelect.getPoint() (both
selectedHandlerNode.targetPointIdSelect.getPoint() and
targetPointIdSelect.getPoint()) before accessing targetPoint.dataType; if
targetPoint is falsy, avoid calling setDataTypes/updatePointsList and handle the
activePointIdSelect.setPointId(activeId) state appropriately (e.g., clear the
points list or return early) so the code does not dereference
targetPoint.dataType.
- Around line 275-282: Found potential runtime errors because
allPoints.find(...) may return undefined; add null guards before using
targetPoint, activePoint, or inactivePoint when building the arrays and passing
parameters into initPointsSelect. Specifically, in the block that sets
selectedHandlerNode.targetPointIdSelect,
selectedHandlerNode.activePointIdSelect, and
selectedHandlerNode.inactivePointIdSelect, ensure targetPoint is checked before
accessing targetPoint.dataType or including it in arrays (e.g., use conditional
expressions or fallback values), and guard activePoint/inactivePoint usage
similarly so initPointsSelect is never called with undefined elements or
targetPoint.dataType when targetPoint is missing.
🧹 Nitpick comments (10)
WebContent/WEB-INF/jsp/eventHandlers.jsp (1)
577-581: Simplify the undefined checks.Comparing with the string
'undefined'is unusual. The$get()function should returnundefined(the value) or an empty string, not the string'undefined'. The current check may not catch actual undefined values.🔎 Proposed simplification
- let activePointId = $get("activePointId"); - activePointId = activePointId == 'undefined' || !activePointId ? 0 : activePointId; + let activePointId = $get("activePointId") || 0; - let inactivePointId = $get("inactivePointId"); - inactivePointId = inactivePointId == 'undefined' || !inactivePointId ? 0 : inactivePointId; + let inactivePointId = $get("inactivePointId") || 0;This handles
undefined,null, empty string, and0consistently by defaulting to0.src/org/scada_lts/web/mvc/api/DataPointApiService.java (3)
303-310: Limit applied before filters may yield fewer results than expected.At line 305,
.limit(searchDataPointJson.getLimit())is applied before thefilterByIncludeIds,filterByExcludeIds, andfilterByDataTypesfilters. This means if the limit is 10 and some of those 10 are filtered out, fewer than 10 results will be returned even if more matching records exist.If the intent is to return up to
limitresults after filtering, the limit should be applied last:🔎 Proposed fix
private static List<DataPointVO> filteredDataPoints(List<DataPointVO> response, SearchDataPointJson searchDataPointJson) { - return response.stream().limit(searchDataPointJson.getLimit()) - .filter(filterByIncludeIds(searchDataPointJson)) + return response.stream() + .filter(filterByIncludeIds(searchDataPointJson)) .filter(filterByExcludeIds(searchDataPointJson)) .filter(filterByDataTypes(searchDataPointJson)) + .limit(searchDataPointJson.getLimit() > 0 ? searchDataPointJson.getLimit() : Long.MAX_VALUE) .collect(Collectors.toList()); }
396-398: Consider matching on extended name for consistency with database search.Line 397 filters by
point.getName(), but the database query ingetDataPointByKeywords(DataPointDAO line 583-588) searches againstCONCAT(ds.name, ' - ', dp.pointName). This inconsistency means admin users (database path) and non-admin users (in-memory filter) may get different results for the same keyword search.🔎 Proposed fix to match extended name
private static Predicate<DataPointVO> filterByKeywords(SearchDataPointJson searchDataPointJson) { - return point -> point.getName() != null && (searchDataPointJson.isStartsWith() ? point.getName().startsWith(searchDataPointJson.getKeywordSearch()) : point.getName().contains(searchDataPointJson.getKeywordSearch())); + return point -> { + String extendedName = point.getExtendedName(); + if (extendedName == null) return false; + String keyword = searchDataPointJson.getKeywordSearch(); + return searchDataPointJson.isStartsWith() + ? extendedName.startsWith(keyword) + : extendedName.contains(keyword); + }; }
288-291: Redundant empty check after validation.Line 281 already validates that
keywordSearchcannot be empty viacheckArgsIfEmptyThenBadRequest. The subsequent check at line 288if(!StringUtils.isEmpty(keywordSearch))is therefore redundant and the code inside will always execute.🔎 Proposed simplification
- if(!StringUtils.isEmpty(keywordSearch)) { - response = searchDataPoint(request, searchDataPointJson, user).stream() - .filter(filterByDataTypes(searchDataPointJson)) - .collect(Collectors.toList()); - } + response = searchDataPoint(request, searchDataPointJson, user).stream() + .filter(filterByDataTypes(searchDataPointJson)) + .collect(Collectors.toList());src/org/scada_lts/mango/adapter/MangoDataPoint.java (1)
102-114: Clarify distinction between overloaded methods.Two methods accept
Set<Integer>IDs:
- Line 102:
getDataPoints(Set<Integer> ids)- returns all matching points- Line 114:
getDataPoints(Set<Integer> pointIds, User user)- filters by user accessConsider renaming the second method to
getDataPointsWithAccess(Set<Integer> pointIds, User user)for consistency with existing naming conventions (e.g.,getDataPointsWithAccess(User user)at line 96) and to clarify the access-filtering behavior.src/org/scada_lts/utils/GetDataPointsUtils.java (3)
90-98: Instantiating services inside utility method creates tight coupling.Lines 94 and 97 create new
EventService()andDataPointService()instances directly. This makes testing difficult and diverges from the dependency injection pattern used elsewhere in this class (where services are passed as parameters).🔎 Proposed fix
-public static Set<DataPointBean> getDataPointsByEventHandlers() { +public static Set<DataPointBean> getDataPointsByEventHandlers(EventService eventService, DataPointService dataPointService) { User user = Common.getUser(); Permissions.ensureAdmin(user); - EventService eventService = new EventService(); - List<EventHandlerVO> eventHandlers = eventService.getEventHandlers(); - return new HashSet<>(GetDataPointsUtils.getDataPointsByEventHandlers(user, eventHandlers, new DataPointService())); + return new HashSet<>(GetDataPointsUtils.getDataPointsByEventHandlers(user, eventHandlers, dataPointService)); }
70-88: Loop-based retrieval performs N+1 queries.
getTargetDataPointsByPointLinksandgetSourceDataPointsByPointLinkscalldataPointService.getDataPoint()inside a loop (lines 73 and 83), resulting in one query per point link. For large lists, this could be slow.Consider collecting all IDs first and fetching them in a single batch query:
🔎 Proposed batch retrieval
public static Set<DataPointBean> getTargetDataPointsByPointLinks(User user, List<PointLinkVO> pointLinks, DataPointService dataPointService) { - Set<DataPointBean> dataPoints = new HashSet<>(); - for (PointLinkVO pointLinkVO : pointLinks) { - DataPointVO targetDataPoint = dataPointService.getDataPoint(pointLinkVO.getTargetPointId()); - if(targetDataPoint != null && GetDataPointsWithAccess.hasDataPointReadPermission(user, targetDataPoint)) - dataPoints.add(new DataPointBean(targetDataPoint)); - } - return dataPoints; + Set<Integer> ids = pointLinks.stream() + .map(PointLinkVO::getTargetPointId) + .collect(Collectors.toSet()); + return dataPointService.getDataPoints(ids).stream() + .filter(point -> GetDataPointsWithAccess.hasDataPointReadPermission(user, point)) + .map(DataPointBean::new) + .collect(Collectors.toSet()); }
127-146: Loop-based retrieval in getDataPointsByEventHandlers also performs N+1 queries.Lines 131-133 call
dataPointService.getDataPoint()three times per event handler inside a loop. This could be optimized by collecting all IDs first and fetching in batch.WebContent/WEB-INF/jsp/dataPointEdit.jsp (1)
86-88: Remove unused variable.The variable
selectedBaseOnExistingPointChooseris assigned but never referenced elsewhere in the code.🔎 Proposed fix
function init() { - let selectedBaseOnExistingPointChooser = initPointsSelect("selected_base_on_existing_point_chooser", "<spring:message code='chosen.selector.selectPoint'/>"); + initPointsSelect("selected_base_on_existing_point_chooser", "<spring:message code='chosen.selector.selectPoint'/>"); }WebContent/resources/common.js (1)
2180-2196: Consider using nullish coalescing for default values.The function uses the
||operator for default values (lines 2183-2193), which treats all falsy values (0,false,"", etc.) as missing. For example, if a caller passesinvisibleEmptyOption: false, the expressioninvisibleEmptyOption || falsestill works correctly, but for consistency and clarity, consider using the nullish coalescing operator (??) which only treatsnullandundefinedas missing.🔎 Proposed refactor
function initPointsSelect(selectHtmlId, placeholderTextSingle, widthPx, excludePointsArray, pointsArray, dataTypes, invisibleEmptyOption, lastEmptyOption) { let ref = {} - ref.excludePointsArray = excludePointsArray || []; + ref.excludePointsArray = excludePointsArray ?? []; ref.limit = 500; ref.selectHtmlId = selectHtmlId; ref.placeholderTextSingle = placeholderTextSingle; - ref.pointsArray = pointsArray || []; - ref.dataTypes = dataTypes || []; + ref.pointsArray = pointsArray ?? []; + ref.dataTypes = dataTypes ?? []; ref.altKey = "id"; ref.altValue = "name"; - ref.widthPx = widthPx || "400px"; - ref.invisibleEmptyOption = invisibleEmptyOption || false; - ref.lastEmptyOption = lastEmptyOption || false; + ref.widthPx = widthPx ?? "400px"; + ref.invisibleEmptyOption = invisibleEmptyOption ?? false; + ref.lastEmptyOption = lastEmptyOption ?? false; return new DataPointsSelect(ref); }Note: This requires ES2020+ support. Verify your browser targets support the nullish coalescing operator.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
WebContent/WEB-INF/jsp/dataPointEdit.jspWebContent/WEB-INF/jsp/dataSourceEdit/editMeta.jspWebContent/WEB-INF/jsp/eventHandlers.jspWebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jspWebContent/WEB-INF/jsp/publisherEdit/editPersistent.jspWebContent/WEB-INF/jsp/reports.jspWebContent/WEB-INF/jsp/scripting.jspWebContent/resources/common.jssrc/com/serotonin/mango/rt/dataSource/meta/MetaPointLocatorRT.javasrc/com/serotonin/mango/rt/dataSource/meta/ScriptExecutor.javasrc/com/serotonin/mango/view/component/ScriptComponent.javasrc/org/scada_lts/dao/DataPointDAO.javasrc/org/scada_lts/mango/adapter/MangoDataPoint.javasrc/org/scada_lts/mango/service/DataPointService.javasrc/org/scada_lts/permissions/migration/dao/OnlyMigrationDataPointService.javasrc/org/scada_lts/utils/GetDataPointsUtils.javasrc/org/scada_lts/web/mvc/api/DataPointApiService.javasrc/org/scada_lts/web/mvc/api/datasources/SearchDataPointJson.java
🚧 Files skipped from review as they are similar to previous changes (3)
- WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jsp
- src/org/scada_lts/web/mvc/api/datasources/SearchDataPointJson.java
- src/org/scada_lts/permissions/migration/dao/OnlyMigrationDataPointService.java
🧰 Additional context used
🧬 Code graph analysis (7)
src/com/serotonin/mango/view/component/ScriptComponent.java (1)
src/com/serotonin/mango/rt/dataSource/meta/ScriptExecutor.java (1)
ScriptExecutor(60-386)
src/org/scada_lts/utils/GetDataPointsUtils.java (2)
src/com/serotonin/mango/Common.java (1)
Common(69-584)src/org/scada_lts/permissions/service/GetDataPointsWithAccess.java (1)
GetDataPointsWithAccess(18-106)
src/org/scada_lts/mango/adapter/MangoDataPoint.java (1)
src/org/scada_lts/dao/model/ScadaObjectIdentifier.java (1)
ScadaObjectIdentifier(14-60)
src/org/scada_lts/web/mvc/api/DataPointApiService.java (3)
src/com/serotonin/mango/web/dwr/beans/DataPointBean.java (1)
DataPointBean(26-111)src/org/scada_lts/dao/model/DataPointIdentifier.java (1)
DataPointIdentifier(6-175)src/org/scada_lts/web/mvc/api/datasources/SearchDataPointJson.java (1)
SearchDataPointJson(9-67)
src/org/scada_lts/dao/DataPointDAO.java (2)
src/org/scada_lts/dao/ViewDAO.java (1)
Deprecated(280-288)src/org/scada_lts/dao/DAO.java (1)
DAO(41-173)
src/org/scada_lts/mango/service/DataPointService.java (4)
src/org/scada_lts/dao/model/ScadaObjectIdentifier.java (1)
ScadaObjectIdentifier(14-60)scadalts-ui/src/store/graphicalViews/index.js (1)
user(223-223)src/org/scada_lts/dao/model/point/PointValueTypeOfREST.java (1)
Deprecated(3-24)src/org/scada_lts/permissions/service/GetDataPointsWithAccess.java (1)
GetDataPointsWithAccess(18-106)
WebContent/resources/common.js (1)
scadalts-ui/src/utils/common.js (2)
i(74-74)getAppLocation(1-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test and Build Scada-LTS new User Interface
- GitHub Check: Compile and Test Scada-LTS application
- GitHub Check: semgrep/ci
🔇 Additional comments (25)
src/com/serotonin/mango/view/component/ScriptComponent.java (3)
98-104: LGTM: Appropriate error severity for invalid scripts.Elevating the log level to ERROR when script validation fails is appropriate, as this represents a critical condition that prevents the script from executing.
151-153: LGTM: Error logging for Undefined result is reasonable.Logging Undefined as an ERROR is appropriate in this rendering context, as scripts are expected to return a displayable value. An Undefined result typically indicates a script that forgot to return a value.
154-157: LGTM: Appropriate error severity for execution exceptions.Elevating the log level to ERROR for script execution exceptions is appropriate, as these represent genuine runtime failures that should be prominently logged for monitoring and debugging.
src/com/serotonin/mango/rt/dataSource/meta/MetaPointLocatorRT.java (1)
300-300: Confirm that ERROR-level logging for these critical failures won't trigger excessive monitoring alerts.These logging statements are appropriately set to ERROR level for critical failure scenarios in a SCADA system:
- Recursion failures that prevent script execution
- Script execution exceptions that block point updates
- Context initialization failures
Verify with your operations team that the ERROR-level logs for these error conditions align with your monitoring and alerting policies, particularly to ensure they don't trigger unnecessary escalations to on-call staff.
WebContent/WEB-INF/jsp/eventHandlers.jsp (3)
253-255: LGTM!The global point select variables are properly declared and will be used to manage per-handler point configurations.
370-441: LGTM!The function properly handles null/undefined points from
getPoint()using conditional checks before adding them to arrays. The logic correctly enables/disables action selects based on whether a valid target point is selected.
763-763: LGTM!The onchange handlers correctly call the new helper functions, and the
disabledattributes on action selects ensure users can only select actions after choosing a valid target point.Also applies to: 771-771, 785-785, 798-798, 812-812
src/org/scada_lts/mango/adapter/MangoDataPoint.java (1)
99-114: LGTM!The new interface methods appropriately extend the data point retrieval capabilities with pagination, exclusion filters, keyword search, and access-aware navigation. The method signatures are well-designed for the performance improvements targeted by this PR.
src/org/scada_lts/utils/GetDataPointsUtils.java (1)
223-229: Past review concern addressed.The
convertToInthelper method now safely handlesNumberFormatExceptionby returning0for unparseable values. This addresses the previously flagged concern aboutInteger.parseIntthrowing exceptions for non-numeric data.However, returning
0as a fallback may cause unintended behavior if0is a valid data point ID. Consider returning a sentinel value like-1or filtering out invalid entries instead.src/org/scada_lts/dao/DataPointDAO.java (4)
194-200: Operator precedence issue addressed.The access filter is now correctly wrapped in parentheses at lines 195 and 199:
"(" + DATA_POINT_FILTERED_BASE_ON_USER_ID_USERS_PROFILE_ID + ") AND "This ensures the
STRCMP/comparison condition applies to the entire permission filter, not just the last OR clause. The previous review concern is resolved.
545-564: SQL construction issue addressed.Line 546 now uses
DATA_POINT_SELECT + " WHERE true "pattern, which provides a valid base for appending additionalANDconditions. This addresses the previously flagged issue about missing WHERE clause.
577-612: SQL injection not possible due to parameterized queries, but consider simplification.The
getDataPointByKeywordsmethod correctly uses parameterized queries with?placeholders and passes arguments separately. The SQL construction is safe.However, the
"?, ".repeat(excludeIds.size() - 1)pattern at line 600 could throw ifexcludeIdsbecomes empty after the initial check (though unlikely due to the guard at line 596).
312-324: Deprecated method retained for backward compatibility.The
@Deprecated(since = "2.8.1")annotation ongetDataPointByKeywordappropriately signals that callers should migrate to the newgetDataPointByKeywordsmethod.src/org/scada_lts/mango/service/DataPointService.java (5)
204-207: LGTM!The new
getDataPointIdentifiersWithAccessmethod correctly delegates to thegetDataPointsWithAccesshelper, maintaining consistent permission checking.
647-652: Consider consistency with existing method at line 643.Lines 643-645 return data points without setting relational data, while other retrieval methods like
getDataPoint(int id)at line 148 callsetRelationalData(dp). This may cause inconsistent behavior depending on which method is called.Verify if callers of
getDataPoints(Set<Integer> pointIds)andgetDataPoints(Set<Integer> pointIds, User user)need relational data (event detectors, comments).
750-761: Navigation methods delegate correctly based on admin status.The
getDataPointIdWithAccessPrevandgetDataPointIdWithAccessNextmethods properly handle the admin vs. non-admin distinction, using simpler queries for admins and access-filtered queries for regular users.
719-726: Keyword parsing differs from deprecated method.Line 722 treats the entire
keywordSearchas a single keyword:Set<String> keywords = Set.of(keywordSearch);But the deprecated
searchDataPointsByat line 713 splits on whitespace:String[] keywords = searchText.split("\\s+");This is a behavioral change that may affect callers. Ensure this is intentional and documented.
241-244: Deprecated annotation added appropriately.The
@Deprecated(since = "2.8.1")annotation correctly markssearchDataPointsfor deprecation, guiding users to the new keyword-based search methods.WebContent/WEB-INF/jsp/publisherEdit/editPersistent.jsp (2)
27-40: LGTM!The context initialization correctly clears any existing context before creating a new
PersistentPointsContextinstance with appropriate configuration from the server response.
62-68: LGTM!The callback correctly synchronizes the points context with the server response after a successful save operation.
WebContent/WEB-INF/jsp/dataSourceEdit/editMeta.jsp (2)
53-66: LGTM!The initialization correctly creates a
ScriptPointsContextinstance with configuration sourced from the locator and contextPoints parameter.
130-130: The onclick handler is now correct.The
addPointToContext()method properly retrieves the select value internally viasuper.getSelectHtmlId()without relying on the image element'sthis.value, which resolves the previous bug.WebContent/WEB-INF/jsp/reports.jsp (1)
65-91: LGTM!The callback correctly extracts the report from
response.data.report, builds a configuration object, clears any existing context, and initializes a newReportPointsContextinstance.WebContent/WEB-INF/jsp/scripting.jsp (2)
115-140: LGTM!The callback correctly extracts data from
response.data, clears any existing context, and initializes a newScriptPointsContextinstance with appropriate configuration.
328-328: Verify thataddPointToContext()retrieves the select value internally.The onclick handler calls
addPointToContext()without arguments. Ensure the method internally retrieves the value from theallPointsListselect element.
- Removed console.log; - Removed PointsContext.clear; - Refactor eventHandlers.jsp, init PointsContext and DataPointsSelect;
- Replace CONCAT_WS with CONCAT; - DataPointApiService.searchDataPoint: Adding a scenario where keywords is empty, then it returns the first sorted points within the limit;
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @src/org/scada_lts/dao/DataPointDAO.java:
- Around line 187-191: The DATA_POINT_NEXT and DATA_POINT_PREV SQL strings use
CONCAT with three args and a trailing LIMIT 1 which is incompatible with
Oracle/SQL Server; update these constants (DATA_POINT_NEXT, DATA_POINT_PREV) to
produce dialect-specific SQL or use an abstraction: replace the three-arg
CONCAT(ds.name, ' - ', dp.pointName) with a portable expression (e.g., nested
CONCAT or string concatenation operator per dialect) and remove LIMIT 1 in favor
of the target DB's pagination (e.g., TOP 1 for SQL Server, ROWNUM/ROW_NUMBER for
Oracle 11g or OFFSET...FETCH for newer engines); ideally implement a small
factory or use Spring JDBC/dialect detection to choose the correct template for
these two constants so the code generates valid SQL for Oracle and SQL Server as
well as MySQL/Postgres.
In @src/org/scada_lts/web/mvc/api/DataPointApiService.java:
- Around line 394-396: filterByIncludeIds currently dereferences
searchDataPointJson.getIncludeIds() and will NPE if includeIds is null; change
filterByIncludeIds(SearchDataPointJson) to first read the includeIds list,
return a no-op predicate (e.g., always true) when includeIds is null, otherwise
return a predicate that checks includeIds.contains(id.getId()), mirroring the
null-safe behavior of filterByExcludeIds and using the SearchDataPointJson and
DataPointVO symbols to locate the change.
In @WebContent/resources/common.js:
- Around line 1334-1374: The sizingField function currently uses Math.random()
to compute a growth step which makes field width changes unpredictable; replace
the random step logic inside sizingField (referencing function sizingField and
initSizeField) with a deterministic fixed increment (e.g., a constant step value
or based on character width) so width increases are consistent, and remove the
debug console.log in isSupported (function isSupported) entirely; ensure no
other debug prints remain and keep behavior of isSupportedFieldSizing unchanged.
In @WebContent/WEB-INF/jsp/eventHandlers.jsp:
- Around line 631-633: The standalone function setAllPoints incorrectly uses
this.allPoints which won't update the intended global allPoints; change it to
assign the global variable directly (e.g., rename the parameter to avoid
shadowing and set global allPoints = points) or remove the function and perform
the assignment inline where the callback is invoked; update references to use
the function name setAllPoints or inline assignment accordingly so the global
allPoints is actually updated.
In @WebContent/WEB-INF/jsp/include/compoundEditor.jsp:
- Around line 308-316: The setDataTypes method on the component iterates
targetPointSelects and currently logs each targetPointSelect to the console;
remove the debug console.log call so setDataTypes only calls
targetPointSelect.setDataTypes(supportedDataTypes) and does not emit console
output (search for the console.log in setDataTypes and delete that line;
optionally scan for other stray debug logs in the same file).
In @WebContent/WEB-INF/jsp/reports.jsp:
- Around line 125-127: setPointsContext currently assigns to this.pointsContext
while saveReport and runReport reference the global pointsContext, which is
fragile; update setPointsContext to assign to the same global binding used by
saveReport/runReport (e.g., pointsContext or window.pointsContext) so all
functions read/write the same variable, or alternatively make all usages
explicitly use a consistent namespace (e.g., change saveReport/runReport to use
this.pointsContext if you convert callers to call setPointsContext with the
correct context); ensure the variable is declared in the intended scope to avoid
accidental globals.
🧹 Nitpick comments (3)
WebContent/WEB-INF/jsp/views.jsp (1)
256-267: DataPointsSelect instances are created but not stored.The
DataPointsSelectinstances are created in the loop but discarded after each iteration. If you need to interact with these selects later (e.g., to retrieve selected values or update options), consider storing them in an object keyed byselectHtmlId.♻️ Suggested refactor if later access is needed
+ var chartComparatorSelects = {}; + function loadPointsSelects() { let pointsSelects = jQuery("select[id^=chartComparator]"); for(let i=0; i < pointsSelects.length; i++) { let ref = {}; ref.selectHtmlId = pointsSelects[i].id; ref.placeholderTextSingle = "<spring:message code='chosen.selector.selectPoint'/>"; ref.widthPx = "200px"; - let dataPointsSelect = new DataPointsSelect(ref); + chartComparatorSelects[pointsSelects[i].id] = new DataPointsSelect(ref); } }WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jsp (1)
24-25: Remove unusedallPointsvariable.The
allPointsarray is declared but never used in this file. It appears to be leftover from the pre-refactor code that has been replaced by thepointsContextapproach.♻️ Proposed cleanup
- var allPoints = new Array(); var pointsContext;src/org/scada_lts/web/mvc/api/DataPointApiService.java (1)
284-304:filterByDataTypesis applied twice whenincludeIdsis non-empty and results exist.When the flow reaches line 291 (
filteredDataPoints), the response has already been filtered byfilterByDataTypesat line 285. ThefilteredDataPointsmethod applies it again at line 302. This is redundant.♻️ Suggested fix: remove duplicate filter from filteredDataPoints
private static List<DataPointVO> filteredDataPoints(List<DataPointVO> response, SearchDataPointJson searchDataPointJson) { return response.stream().limit(searchDataPointJson.getLimit()) .filter(filterByIncludeIds(searchDataPointJson)) .filter(filterByExcludeIds(searchDataPointJson)) - .filter(filterByDataTypes(searchDataPointJson)) .collect(Collectors.toList()); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
WebContent/WEB-INF/jsp/dataPointDetails.jspWebContent/WEB-INF/jsp/dataPointEdit.jspWebContent/WEB-INF/jsp/dataSourceEdit/editMeta.jspWebContent/WEB-INF/jsp/eventHandlers.jspWebContent/WEB-INF/jsp/include/compoundEditor.jspWebContent/WEB-INF/jsp/include/settingsEditor.jspWebContent/WEB-INF/jsp/pointEdit/pointName.jspWebContent/WEB-INF/jsp/pointLinks.jspWebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jspWebContent/WEB-INF/jsp/publisherEdit/editPachube.jspWebContent/WEB-INF/jsp/publisherEdit/editPersistent.jspWebContent/WEB-INF/jsp/reports.jspWebContent/WEB-INF/jsp/scripting.jspWebContent/WEB-INF/jsp/views.jspWebContent/resources/common.jssrc/org/scada_lts/dao/DataPointDAO.javasrc/org/scada_lts/mango/service/DataPointService.javasrc/org/scada_lts/web/mvc/api/DataPointApiService.javasrc/org/scada_lts/web/mvc/api/datasources/SearchDataPointJson.java
🚧 Files skipped from review as they are similar to previous changes (4)
- WebContent/WEB-INF/jsp/dataSourceEdit/editMeta.jsp
- WebContent/WEB-INF/jsp/dataPointDetails.jsp
- WebContent/WEB-INF/jsp/dataPointEdit.jsp
- src/org/scada_lts/web/mvc/api/datasources/SearchDataPointJson.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-06T22:12:13.613Z
Learnt from: Limraj
Repo: SCADA-LTS/Scada-LTS PR: 3239
File: WebContent/WEB-INF/jsp/eventHandlers.jsp:275-282
Timestamp: 2026-01-06T22:12:13.613Z
Learning: In WebContent/WEB-INF/jsp/eventHandlers.jsp, when editing an existing event handler in showHandlerEdit(), targetPoint being null/undefined is considered an invalid state (data integrity violation) because if a handler exists with a targetPointId, the corresponding point is guaranteed to exist in allPoints. The maintainer has asserted this architectural invariant.
Applied to files:
WebContent/WEB-INF/jsp/views.jspWebContent/WEB-INF/jsp/pointEdit/pointName.jspWebContent/WEB-INF/jsp/publisherEdit/editPachube.jspWebContent/WEB-INF/jsp/publisherEdit/editPersistent.jspWebContent/WEB-INF/jsp/include/settingsEditor.jspWebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jspWebContent/WEB-INF/jsp/include/compoundEditor.jspWebContent/WEB-INF/jsp/reports.jspWebContent/WEB-INF/jsp/pointLinks.jspWebContent/WEB-INF/jsp/eventHandlers.jspsrc/org/scada_lts/web/mvc/api/DataPointApiService.java
🧬 Code graph analysis (3)
src/org/scada_lts/mango/service/DataPointService.java (5)
src/org/scada_lts/serorepl/utils/StringUtils.java (1)
StringUtils(12-194)src/org/scada_lts/dao/model/ScadaObjectIdentifier.java (1)
ScadaObjectIdentifier(14-60)scadalts-ui/src/store/graphicalViews/index.js (1)
user(223-223)src/org/scada_lts/dao/model/point/PointValueTypeOfREST.java (1)
Deprecated(3-24)src/org/scada_lts/permissions/service/GetDataPointsWithAccess.java (1)
GetDataPointsWithAccess(18-106)
WebContent/resources/common.js (3)
WebContent/resources/header.js (1)
imgNode(34-34)scadalts-ui/src/utils/common.js (2)
i(74-74)getAppLocation(1-12)scadalts-ui/src/store/dataPoint/index.js (1)
data(122-122)
src/org/scada_lts/dao/DataPointDAO.java (1)
src/org/scada_lts/dao/DAO.java (1)
DAO(41-173)
🔇 Additional comments (48)
WebContent/WEB-INF/jsp/pointEdit/pointName.jsp (2)
29-29: LGTM - Good defensive practice.Adding
return false;prevents unintended default form submission whendoSaveis invoked from event handlers, ensuring the save flow executes through the intended chain (text renderer → event text renderer → chart renderer → point event detectors → final submit).
52-58: DataPointsSelect correctly implements dynamic point loading.The
pointsArrayinitialization with only the current point is an intentional performance optimization.DataPointsSelectfetches available points dynamically through AJAX (api/datapoints/bean) in two scenarios:
- On initialization (empty search) to populate the full dropdown list
- On user search input (with 700ms debounce) to filter points by keyword
This design improves initial page load performance while maintaining full dropdown functionality for navigation and search, directly addressing the PR's "very slow editing" issue.
WebContent/WEB-INF/jsp/views.jsp (1)
315-319: LGTM!The document ready initialization is appropriate for ensuring DOM elements exist before creating the DataPointsSelect instances.
WebContent/WEB-INF/jsp/publisherEdit/editPersistent.jsp (5)
22-35: LGTM!The initialization correctly creates a
PersistentPointsContextwith the proper configuration object and assigns it to the globalpointsContextvariable. The ref object includes all necessary properties for context-driven point management.
38-40: LGTM!The
selectPointfunction correctly delegates topointsContext.addPointToContext(), aligning with the new context-driven architecture.
49-53: LGTM!The save implementation correctly uses
pointsContext.convertToSave()and routes to the newsavePersistentCBcallback.
56-62: LGTM!The callback properly chains to
savePublisherCBand then synchronizes the context state only when there are no error messages. This ensures the UI reflects the server-confirmed state.
128-128: LGTM!Adding the XID column header aligns with the context-based representation that now includes point XIDs.
WebContent/WEB-INF/jsp/reports.jsp (5)
26-26: LGTM!Global
pointsContextvariable declaration aligns with the context-driven pattern used across the codebase.
65-83: LGTM!The
loadReportCBcorrectly extracts report data fromresponse.data.report, builds a proper ref object forReportPointsContext, and callssetPointsContextto store it.
297-297: LGTM!Using
pointsContext.convertToSave()aligns with the context-driven approach.
368-368: LGTM!Consistent use of
pointsContext.convertToSave()for the run report flow.
476-476: LGTM!The onclick handler correctly calls
pointsContext.addPointToContext()without passingthis.value, addressing the previously identified issue.WebContent/WEB-INF/jsp/publisherEdit/editPachube.jsp (5)
22-34: LGTM!The initialization correctly creates a
PachubePointsContextwith the proper configuration, consistent with the pattern in other publisher edit JSPs.
37-39: LGTM!The
selectPointfunction properly delegates to the context.
49-53: LGTM!The save implementation correctly uses
pointsContext.convertToSave()(thethis.pointsContextissue from past review has been addressed).
56-62: LGTM!The callback properly chains to
savePublisherCBand synchronizes context state on success.
118-118: LGTM!Adding the XID column header is consistent with other publisher edit pages.
WebContent/WEB-INF/jsp/include/settingsEditor.jsp (5)
76-81: LGTM!The
DataPointsSelectinitialization within theSettingsEditorconstructor properly sets up the internal helper with the correct select element ID and placeholder text.
87-108: LGTM!The
openmethod correctly handles the new response structure fromViewDwr.getViewComponentRes, extractingresponse.data.compandresponse.data.pointList, and properly initializes the component state.
124-124: LGTM!Using
this.dataPointsSelect.getPointId()is correct within the object method context.
137-145: LGTM!The delegation to
dataPointsSelectfor setting points and data types properly encapsulates the point management logic.
147-165: Verify: Narrowing pointsArray to single point after selection.After a point is selected (lines 157-160),
setPointsArray([point])restricts the dropdown to only that point. This means users cannot change their selection to a different point without reopening the editor. Is this intentional behavior to lock in the selection?If this is unintentional, consider preserving the original points list:
if(point) { - this.dataPointsSelect.setPointsArray([point]); this.dataPointsSelect.setPointId(point.id); }WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jsp (5)
33-57: LGTM!The initialization correctly creates a
SenderPointsContextwith the proper configuration, consistent with the pattern in other publisher edit JSPs.
170-172: LGTM!The
selectPointfunction properly delegates to the context.
180-180: LGTM!Correctly uses
pointsContext.convertToSave()(thethis.pointsContextissue from past review has been addressed).
190-197: LGTM!The callback properly chains to
savePublisherCB, refreshes static headers, and synchronizes context state on success.
408-408: LGTM!Adding the XID column header is consistent with other publisher edit pages.
WebContent/WEB-INF/jsp/pointLinks.jsp (7)
27-28: LGTM!Global variables for the
DataPointsSelectinstances allow proper state management and access across functions.
30-42: LGTM!The initialization correctly iterates over
response.data.pointLinksand callsupdatePointLinkwith the appropriate data structure.
44-79: LGTM!The
showPointLinkfunction correctly createsDataPointsSelectinstances with exclusion lists to prevent selecting the same point as both source and target, and properly initializes the selected values.
81-120: LGTM!The client-side validation for undefined source/target points with proper error messaging is a good addition. Using
createValidationMessageandshowDwrMessagesprovides consistent user feedback.
141-147: LGTM!The
getPointNamefunction correctly iterates the provided points array to find and return the matching point name.
165-181: LGTM!The change handlers correctly update data type constraints and exclusion lists bidirectionally, preventing invalid point link configurations.
229-234: LGTM!The onchange handlers are properly wired to trigger the bidirectional filtering logic.
WebContent/resources/common.js (1)
1996-1996: URL encoding properly implemented.The URL construction now correctly uses
encodeURIComponent(keywordSearch)to safely encode the search parameter. This addresses the previously flagged issue with special character handling.src/org/scada_lts/dao/DataPointDAO.java (4)
548-567: LGTM!The query construction correctly uses
WHERE trueas a base, addressing the previous issue. TheexcludeIdsare properly parameterized. The direct append ofLIMIT/OFFSETintegers is safe from SQL injection.
569-578: LGTM!The method correctly handles the empty set case and constructs a parameterized IN clause. The ORDER BY ensures consistent result ordering regardless of set iteration order.
580-614: LGTM with portability caveat.The keyword search is properly parameterized. The same
CONCATportability concern applies here as noted forDATA_POINT_NEXT/DATA_POINT_PREVconstants.
616-650: LGTM!The navigation methods correctly implement access-controlled prev/next lookups. The parentheses fix from the past review is properly applied in the SQL constants (lines 194-200), ensuring the
ANDcondition applies to the entire permission filter.src/org/scada_lts/web/mvc/api/DataPointApiService.java (2)
297-304: Limit applied before filtering may return fewer results than expected.At line 299,
.limit()is applied beforefilterByIncludeIdsandfilterByExcludeIds. If the limit is 10 and 3 items are filtered out, only 7 results are returned. If this is intentional for pagination purposes, consider documenting the behavior. Otherwise, apply limit after filtering.
267-275: LGTM!These wrapper methods correctly delegate to
searchDataPointand map results to the appropriate DTOs.src/org/scada_lts/mango/service/DataPointService.java (6)
204-207: LGTM!Clean delegation to the access-aware retrieval pattern.
642-652: LGTM!The overloads correctly delegate to the DAO and apply access filtering when a user is provided.
719-727: Behavioral change: keyword search no longer splits on whitespace.The deprecated
searchDataPointsBy(line 711-717) splitssearchTexton\\s+to search for multiple keywords. This new method wraps the entirekeywordSearchstring inSet.of(), so "foo bar" searches for the literal string "foo bar" rather than "foo" AND "bar".Verify this is the intended behavior change.
729-739: LGTM!The method correctly delegates to the DAO and applies optional relational data loading and sorting.
741-748: LGTM!Clean extension of the existing access-aware retrieval with optional relational data loading.
750-762: LGTM!The navigation methods correctly branch on admin status and delegate to the appropriate DAO methods with proper parameters.
- Used CONCAT with two arguments; (DataPointDAO.java)
|
@coderabbitai review |
- Corrected DataPointDAO.getDataPointByKeywords - insensitive case;
- Corrected DataPointDAO.filterByKeywords - insensitive case; - Fixed check getExtendedName no getName;
- Corrected name method DataPointApiService.filteringAndPaginationPoints;
- Reduce limit to 200;
- Removed updated list if size < 10;
- Removed exclude by dataType for target point links;
- Corrected relation selects in pointLinks and eventHandlers, filtering by settable, setPermissionRequired
- Corrected BACnetIPDataSourceRT: Added check SegmentationSupported parameter from RemoteDevice, until this field is set, it will try to get the remote device;
- Set limit 500;
…x/#3242_Fixed_BACnet_Read
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/org/scada_lts/permissions/service/GetDataSourcesWithAccess.java (1)
50-75: Potential permission regression for users with point-only access.Line 52 routes through the new helper that checks only
Permissions.hasDataSourcePermission(), which excludes point-level permissions. However,selectDataSourcesWithAccess()in the DAO layer (via SQL join and WHERE clause on dataPoints) still includes data sources accessible through point-level permissions. This creates an inconsistency: users with only point-level permissions will see data sources in list views but fail permission checks when attempting to access them. The deprecated overload (line 78) provided a fallback via DataPointDAO that is no longer active. Confirm this access tightening is intentional or restore point-level permission consideration in hasReadPermission().
🤖 Fix all issues with AI agents
In `@src/org/scada_lts/dao/DataPointDAO.java`:
- Around line 549-567: The query in getDataPointsWithLimit builds a non-portable
SQL string (templateSelectWhereId) using "WHERE true" and MySQL/Postgres
LIMIT/OFFSET; change "WHERE true" to "WHERE 1=1" and remove hard-coded
"LIMIT"/"OFFSET" clauses, and instead apply dialect-aware pagination or use the
JDBC API to limit rows: either (A) detect the DB dialect and append the
appropriate pagination syntax (Oracle ROWNUM or FETCH/OFFSET, SQL Server
OFFSET/FETCH) before executing DAO.getInstance().getJdbcTemp().query(...), or
(B) avoid SQL pagination and call the JDBC/templating layer with
setMaxRows()/setFetchSize to enforce limit/offset in a DB-agnostic way; update
getDataPointsWithLimit (and the similar method that uses templateSelectWhereId
at line ~582) to use the chosen approach and ensure args remain the prepared
parameters passed to query().
In `@src/org/scada_lts/utils/GetDataPointsUtils.java`:
- Around line 150-156: The current getDataPointsByChildren maps non-numeric
childPointIds to 0 via convertToInt, causing bogus id lookups; change the
pipeline so it only collects valid numeric IDs instead of coercing invalid
values to 0: replace the map(convertToInt) step with a parse-and-filter approach
(e.g., tryParse/regex/Integer.parseInt inside a try/catch) to produce only valid
Integer values and filter out invalid ones before collecting into the Set, and
update or replace convertToInt (and its other call sites referenced around lines
224-229) to either return an Optional<Integer> or provide a fail-safe parse
helper so all callers skip invalid IDs rather than using 0.
In `@src/org/scada_lts/web/mvc/api/DataPointApiService.java`:
- Around line 278-285: Validate that pagination parameters in searchDataPoint
are non-negative before applying stream skip/limit: check
searchDataPointJson.getPage() and searchDataPointJson.getLimit() (and any
equivalent variables in the overloaded searchDataPoint method) and if either is
negative throw a Bad Request (HTTP 400) error or return a 400 response rather
than allowing stream.skip/limit to receive negative values; update both the
searchDataPoint(HttpServletRequest, SearchDataPointJson) entrypoint and the
internal searchDataPoint(...) helper to perform this guard and include a clear
error message referencing the invalid parameter.
- Around line 427-430: The predicate in DataPointApiService.filterByIncludeIds
treats an empty includeIds set as "filter everything"; change the condition so
an empty set is a no-op: retrieve includeIds via
searchDataPointJson.getIncludeIds() and return a predicate that allows all when
includeIds is null or includeIds.isEmpty(), otherwise check
includeIds.contains(dataPoint.getId()); update the condition in
filterByIncludeIds accordingly.
In `@WebContent/WEB-INF/jsp/eventHandlers.jsp`:
- Around line 501-514: The code currently uses truthy checks and clears provided
zero/empty values when selectedHandlerNode is null; change the logic in
targetPointSelectChanged (references: selectedHandlerNode, activeValueStr,
inactiveValueStr, EventHandlersDwr.createSetValueContent) to use nullish checks
(?? or explicit null/undefined checks) so activeValueStr = (activeValue ??
(selectedHandlerNode ? selectedHandlerNode.object.activeValueToSet : "")) and
similarly for inactiveValueStr; also update the else branch that calls
EventHandlersDwr.createSetValueContent to pass (activeValue ?? "") and
(inactiveValue ?? "") instead of always ""; apply the same fixes for the other
occurrence around the 687-692 block.
🧹 Nitpick comments (2)
src/org/scada_lts/mango/adapter/MangoDataPoint.java (1)
99-109: Clarify new API semantics via Javadoc (paging/access/ordering).The new overloads introduce ambiguous parameters (e.g.,
start) and access-sensitive behavior (includeRelationalData,prev/nextordering). Please add Javadoc describing expected matching rules, paging semantics (offset/limit), ordering used for prev/next, and when access filtering applies to avoid inconsistent implementations.WebContent/WEB-INF/jsp/pointLinks.jsp (1)
139-145: Consider renaming the parameter for clarity.The parameter
sourcePointsis misleading since this function is called with bothdata.sourcePointsanddata.targetPoints(line 135). Renaming it topointsorpointListwould better reflect its generic usage.♻️ Suggested rename
- function getPointName(pointId, sourcePoints) { - for (var i=0; i<sourcePoints.length; i++) { - if (sourcePoints[i].id == pointId) - return sourcePoints[i].name; + function getPointName(pointId, points) { + for (var i=0; i<points.length; i++) { + if (points[i].id == pointId) + return points[i].name; } return null; }
| public List<DataPointVO> searchDataPoint(HttpServletRequest request, | ||
| SearchDataPointJson searchDataPointJson) { | ||
|
|
||
| checkIfLogoutThenUnauthorized(request); | ||
| User user = Common.getUser(request); | ||
| Set<Integer> includeIds = searchDataPointJson.getIncludeIds(); | ||
|
|
||
| List<DataPointVO> response = searchDataPoint(request, searchDataPointJson, user); |
There was a problem hiding this comment.
Guard against negative page/limit before stream pagination.
skip/limit throw on negative values. If clients send -1, this becomes a 500 instead of a 400.
✅ Suggested fix
public List<DataPointVO> searchDataPoint(HttpServletRequest request,
SearchDataPointJson searchDataPointJson) {
checkIfLogoutThenUnauthorized(request);
User user = Common.getUser(request);
+ if (searchDataPointJson.getPage() < 0 || searchDataPointJson.getLimit() < 0) {
+ throw new BadRequestException("page and limit must be >= 0", request.getRequestURI());
+ }
Set<Integer> includeIds = searchDataPointJson.getIncludeIds();Also applies to: 339-344
🤖 Prompt for AI Agents
In `@src/org/scada_lts/web/mvc/api/DataPointApiService.java` around lines 278 -
285, Validate that pagination parameters in searchDataPoint are non-negative
before applying stream skip/limit: check searchDataPointJson.getPage() and
searchDataPointJson.getLimit() (and any equivalent variables in the overloaded
searchDataPoint method) and if either is negative throw a Bad Request (HTTP 400)
error or return a 400 response rather than allowing stream.skip/limit to receive
negative values; update both the searchDataPoint(HttpServletRequest,
SearchDataPointJson) entrypoint and the internal searchDataPoint(...) helper to
perform this guard and include a clear error message referencing the invalid
parameter.
| if (selectedHandlerNode) { | ||
| activeValueStr = activeValue ? activeValue : selectedHandlerNode.object.activeValueToSet; | ||
| inactiveValueStr = inactiveValue ? inactiveValue : selectedHandlerNode.object.inactiveValueToSet; | ||
|
|
||
| EventHandlersDwr.createSetValueContent(targetPointId, activeValueStr, "Active", | ||
| function(content) { $("activeValueToSetContent").innerHTML = content; }); | ||
| EventHandlersDwr.createSetValueContent(targetPointId, inactiveValueStr, "Inactive", | ||
| function(content) { $("inactiveValueToSetContent").innerHTML = content; }); | ||
|
|
||
| } else { | ||
| EventHandlersDwr.createSetValueContent(targetPointId, "", "Active", | ||
| function(content) { $("activeValueToSetContent").innerHTML = content; }); | ||
| EventHandlersDwr.createSetValueContent(targetPointId, "", "Inactive", | ||
| function(content) { $("inactiveValueToSetContent").innerHTML = content; }); |
There was a problem hiding this comment.
Preserve set‑point values after save (and for falsy values).
When creating a new handler, selectedHandlerNode is still null during the save callback, so targetPointSelectChanged(...) ignores the provided activeValue/inactiveValue and resets the UI to empty. Also, the truthy checks drop valid 0/"" values. Please use nullish checks and apply provided values even when selectedHandlerNode is null.
🐛 Proposed fix
- if (selectedHandlerNode) {
- activeValueStr = activeValue ? activeValue : selectedHandlerNode.object.activeValueToSet;
- inactiveValueStr = inactiveValue ? inactiveValue : selectedHandlerNode.object.inactiveValueToSet;
-
- EventHandlersDwr.createSetValueContent(targetPointId, activeValueStr, "Active",
- function(content) { $("activeValueToSetContent").innerHTML = content; });
- EventHandlersDwr.createSetValueContent(targetPointId, inactiveValueStr, "Inactive",
- function(content) { $("inactiveValueToSetContent").innerHTML = content; });
-
- } else {
- EventHandlersDwr.createSetValueContent(targetPointId, "", "Active",
- function(content) { $("activeValueToSetContent").innerHTML = content; });
- EventHandlersDwr.createSetValueContent(targetPointId, "", "Inactive",
- function(content) { $("inactiveValueToSetContent").innerHTML = content; });
- }
+ const fallbackActive = selectedHandlerNode ? selectedHandlerNode.object.activeValueToSet : "";
+ const fallbackInactive = selectedHandlerNode ? selectedHandlerNode.object.inactiveValueToSet : "";
+ activeValueStr = (activeValue ?? fallbackActive);
+ inactiveValueStr = (inactiveValue ?? fallbackInactive);
+
+ EventHandlersDwr.createSetValueContent(targetPointId, activeValueStr, "Active",
+ function(content) { $("activeValueToSetContent").innerHTML = content; });
+ EventHandlersDwr.createSetValueContent(targetPointId, inactiveValueStr, "Inactive",
+ function(content) { $("inactiveValueToSetContent").innerHTML = content; });Also applies to: 687-692
🤖 Prompt for AI Agents
In `@WebContent/WEB-INF/jsp/eventHandlers.jsp` around lines 501 - 514, The code
currently uses truthy checks and clears provided zero/empty values when
selectedHandlerNode is null; change the logic in targetPointSelectChanged
(references: selectedHandlerNode, activeValueStr, inactiveValueStr,
EventHandlersDwr.createSetValueContent) to use nullish checks (?? or explicit
null/undefined checks) so activeValueStr = (activeValue ?? (selectedHandlerNode
? selectedHandlerNode.object.activeValueToSet : "")) and similarly for
inactiveValueStr; also update the else branch that calls
EventHandlersDwr.createSetValueContent to pass (activeValue ?? "") and
(inactiveValue ?? "") instead of always ""; apply the same fixes for the other
occurrence around the 687-692 block.
There was a problem hiding this comment.
There's an if else, so what you wrote doesn't seem to make sense. Try rephrasing it if I'm wrong.
There was a problem hiding this comment.
@Limraj, you're right to be confused by the explanation—let me clarify what's wrong with the current if-else block:
Two separate issues:
-
In the
if (selectedHandlerNode)branch (lines 501-503):- Uses truthy checks:
activeValue ? activeValue : ... - This drops valid falsy values like
0or""(empty string) - Should use nullish coalescing (
??) instead
- Uses truthy checks:
-
In the
elsebranch (lines 510-514):- Always passes
""tocreateSetValueContent, completely ignoring theactiveValue/inactiveValueparameters - This is a problem when creating a new handler—after the save callback returns,
selectedHandlerNodeis still null/just assigned, but you want to display the saved values (not empty strings)
- Always passes
The proposed fix consolidates the logic:
// Compute fallback once (works whether selectedHandlerNode exists or not)
const fallbackActive = selectedHandlerNode ? selectedHandlerNode.object.activeValueToSet : "";
const fallbackInactive = selectedHandlerNode ? selectedHandlerNode.object.inactiveValueToSet : "";
// Use nullish coalescing to respect 0/"" and provided parameters
activeValueStr = (activeValue ?? fallbackActive);
inactiveValueStr = (inactiveValue ?? fallbackInactive);
// Call once (no duplication)
EventHandlersDwr.createSetValueContent(targetPointId, activeValueStr, "Active", ...);
EventHandlersDwr.createSetValueContent(targetPointId, inactiveValueStr, "Inactive", ...);This way, the provided activeValue/inactiveValue parameters are always respected (even when selectedHandlerNode is null), and valid falsy values like 0 are preserved.
Does this clarification make sense?
🧠 Learnings used
Learnt from: Limraj
Repo: SCADA-LTS/Scada-LTS PR: 3239
File: WebContent/WEB-INF/jsp/eventHandlers.jsp:275-282
Timestamp: 2026-01-06T22:12:18.647Z
Learning: In WebContent/WEB-INF/jsp/eventHandlers.jsp, when editing an existing event handler in showHandlerEdit(), targetPoint being null/undefined is considered an invalid state (data integrity violation) because if a handler exists with a targetPointId, the corresponding point is guaranteed to exist in allPoints. The maintainer has asserted this architectural invariant.
Learnt from: Limraj
Repo: SCADA-LTS/Scada-LTS PR: 3239
File: src/br/org/scadabr/view/component/ChartComparatorComponent.java:63-66
Timestamp: 2026-01-12T08:45:00.446Z
Learning: In ChartComparatorComponent, data point select dropdowns are intentionally rendered with Collections.emptyList() and populated client-side using lazy loading via the DataPointsSelect JavaScript class, initialized by loadPointsSelects("select[id^=chartComparator]", ...) in viewEdit.jsp and views.jsp.
There was a problem hiding this comment.
If there is an if else, does it execute one or the other block and not simultaneously?
There was a problem hiding this comment.
@Limraj, you're absolutely correct—an if-else executes only one branch, not both simultaneously.
Let me clarify the confusion in my previous explanation:
The issue isn't that both branches execute at once. Instead, different problems occur at different times depending on which path executes:
-
When editing an existing handler (line 501):
selectedHandlerNodeexists →ifbranch executes- Problem:
activeValue ? activeValue : ...drops valid falsy values like0or""
-
When creating a new handler and the save callback runs (line 510):
selectedHandlerNodeis still null →elsebranch executes- Problem: always passes
""tocreateSetValueContent, completely ignoring theactiveValue/inactiveValueparameters returned from the server
The proposed fix consolidates both paths into a single logical flow that:
- Respects the provided parameters (
activeValue/inactiveValue) whetherselectedHandlerNodeexists or not - Uses nullish coalescing (
??) to preserve valid falsy values like0or""
Does this make sense now?
🧠 Learnings used
Learnt from: Limraj
Repo: SCADA-LTS/Scada-LTS PR: 3239
File: WebContent/WEB-INF/jsp/eventHandlers.jsp:275-282
Timestamp: 2026-01-06T22:12:18.647Z
Learning: In WebContent/WEB-INF/jsp/eventHandlers.jsp, when editing an existing event handler in showHandlerEdit(), targetPoint being null/undefined is considered an invalid state (data integrity violation) because if a handler exists with a targetPointId, the corresponding point is guaranteed to exist in allPoints. The maintainer has asserted this architectural invariant.
Learnt from: Limraj
Repo: SCADA-LTS/Scada-LTS PR: 3239
File: src/br/org/scadabr/view/component/ChartComparatorComponent.java:63-66
Timestamp: 2026-01-12T08:45:00.446Z
Learning: In ChartComparatorComponent, data point select dropdowns are intentionally rendered with Collections.emptyList() and populated client-side using lazy loading via the DataPointsSelect JavaScript class, initialized by loadPointsSelects("select[id^=chartComparator]", ...) in viewEdit.jsp and views.jsp.
There was a problem hiding this comment.
There is really no error here, it's just a matter of refactoring to avoid code duplication.
There was a problem hiding this comment.
@Limraj, you're absolutely right—I apologize for overstating the issue. If the code works correctly in your domain (e.g., the values passed are never problematic falsy values, or the else branch is only reached in contexts where "" is the correct default), then it's indeed just a refactoring opportunity to reduce duplication rather than a functional bug.
Thank you for the clarification, and I'll defer to your judgment on whether the refactoring is worth it for your codebase!
🧠 Learnings used
Learnt from: Limraj
Repo: SCADA-LTS/Scada-LTS PR: 3239
File: WebContent/WEB-INF/jsp/eventHandlers.jsp:275-282
Timestamp: 2026-01-06T22:12:18.647Z
Learning: In WebContent/WEB-INF/jsp/eventHandlers.jsp, when editing an existing event handler in showHandlerEdit(), targetPoint being null/undefined is considered an invalid state (data integrity violation) because if a handler exists with a targetPointId, the corresponding point is guaranteed to exist in allPoints. The maintainer has asserted this architectural invariant.
Learnt from: Limraj
Repo: SCADA-LTS/Scada-LTS PR: 3239
File: src/br/org/scadabr/view/component/ChartComparatorComponent.java:63-66
Timestamp: 2026-01-12T08:45:00.446Z
Learning: In ChartComparatorComponent, data point select dropdowns are intentionally rendered with Collections.emptyList() and populated client-side using lazy loading via the DataPointsSelect JavaScript class, initialized by loadPointsSelects("select[id^=chartComparator]", ...) in viewEdit.jsp and views.jsp.
- Corrected validation SearchDataPointJson; - Corrected where true on where 1=1;
…raphical_chart' into fix/#3242_Fixed_BACnet_Read
- Corrected editPointCB in dataSourceEdit;
…raphical_chart' into fix/#3242_Fixed_BACnet_Read
#3242 Fixed BACnet Read:
- Corrected bean validation directive, use @PositiveOrZero;
- Corrected logging ajax error ;
- Refactoring DataPointsSelect: inputSelector, listSelector, removed setPermissionRequired from public api; - Corrected settable for event handler target;
- Rename parameter in SearchDataPointJson settable to pointSettable
- Fixed reports and scripts;
- Revert GetDataSourcesWithAccess;
- Revert GetDataSourcesWithAccess;
- Corrected UsersApiService;
- Fixed EnhancedImageChartComponent;
- Fixed localization for add point to context; - Default DataPointsSelect.selectHtmlId = "allPointsList"; - Added method PointLinksDwr.getPointLinkResponse;
- Refactor classes: PointsContext, DataPointsSelect (common.js) - Generalization of the object select component with lazy loading, created ObjectsSelect class;
Summary by CodeRabbit
New Features
Improvements
Localization
✏️ Tip: You can customize this high-level summary in your review settings.