Skip to content

reusable xml id validation method#4584

Open
Jimmy-Joseph19 wants to merge 4 commits intomainfrom
shared-xml-id-validation
Open

reusable xml id validation method#4584
Jimmy-Joseph19 wants to merge 4 commits intomainfrom
shared-xml-id-validation

Conversation

@Jimmy-Joseph19
Copy link
Copy Markdown
Contributor

Add validateId method to project-access for reusability across multiple packages

@Jimmy-Joseph19 Jimmy-Joseph19 requested review from a team as code owners April 22, 2026 06:59
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 22, 2026

🦋 Changeset detected

Latest commit: f8e83d3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 52 packages
Name Type
@sap-ux/project-access Patch
@sap-ux/fe-fpm-writer Patch
@sap-ux/abap-deploy-config-sub-generator Patch
@sap-ux/abap-deploy-config-writer Patch
@sap-ux/adp-flp-config-sub-generator Patch
@sap-ux/adp-tooling Patch
@sap-ux/annotation-generator Patch
@sap-ux/app-config-writer Patch
@sap-ux/cap-config-writer Patch
@sap-ux/cf-deploy-config-sub-generator Patch
@sap-ux/cf-deploy-config-writer Patch
@sap-ux/create Patch
@sap-ux/deploy-config-sub-generator Patch
@sap-ux/environment-check Patch
@sap-ux/eslint-plugin-fiori-tools Patch
@sap-ux/fiori-annotation-api Patch
@sap-ux/fiori-app-sub-generator Patch
@sap-ux/fiori-elements-writer Patch
@sap-ux/fiori-generator-shared Patch
@sap-ux/flp-config-inquirer Patch
@sap-ux/flp-config-sub-generator Patch
@sap-ux/generator-adp Patch
@sap-ux/inquirer-common Patch
@sap-ux/launch-config Patch
@sap-ux/mockserver-config-writer Patch
@sap-ux/odata-service-inquirer Patch
@sap-ux/odata-service-writer Patch
@sap-ux/preview-middleware Patch
@sap-ux/project-input-validator Patch
@sap-ux/project-integrity Patch
@sap-ux/repo-app-import-sub-generator Patch
@sap-ux/telemetry Patch
@sap-ux/ui5-application-inquirer Patch
@sap-ux/ui5-library-reference-inquirer Patch
@sap-ux/ui5-library-reference-sub-generator Patch
@sap-ux/ui5-library-reference-writer Patch
@sap-ux/ui5-library-writer Patch
@sap-ux/ui5-test-writer Patch
@sap-ux-private/adaptation-editor-tests Patch
@sap-ux/fe-fpm-cli Patch
@sap-ux/backend-proxy-middleware-cf Patch
@sap-ux/fiori-freestyle-writer Patch
@sap-ux-private/preview-middleware-client Patch
@sap-ux/abap-deploy-config-inquirer Patch
@sap-ux/deploy-config-generator-shared Patch
@sap-ux/ui-service-sub-generator Patch
@sap-ux/ui5-library-sub-generator Patch
@sap-ux/cf-deploy-config-inquirer Patch
@sap-ux/deploy-tooling Patch
@sap-ux/ui-service-inquirer Patch
@sap-ux/ui5-library-inquirer Patch
@sap-ux/generator-simple-fe Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@hyperspace-insights
Copy link
Copy Markdown
Contributor

Summary

The following content is AI-generated and provides a summary of the pull request:


Add Reusable validateId Method to project-access

New Feature

✨ Introduces a reusable validateId function in @sap-ux/project-access that checks whether an XML element ID is unique across view and fragment files. This consolidates duplicate ID-checking logic previously scattered across packages.

The new function supports two overloads:

  • Synchronous: when XML file contents are provided directly as strings
  • Asynchronous: when an appPath is provided to scan the filesystem for .view.xml and .fragment.xml files (with optional mem-fs-editor support)

Changes

  • packages/project-access/src/library/helpers.ts: Added the validateId function with synchronous and asynchronous overloads. Uses fast-xml-parser for XML parsing and recursively searches for element IDs. Also added findFilesByExtension and mem-fs-editor imports.
  • packages/project-access/src/library/index.ts: Exported validateId from the library index.
  • packages/fe-fpm-writer/src/common/file.ts: Replaced the local checkElementIdAvailable implementation (which used @xmldom/xmldom's DOMParser) with a call to the new shared validateId from @sap-ux/project-access, removing the DOMParser dependency.
  • packages/project-access/test/library/helpers.test.ts: Added a comprehensive validateId test suite covering ID existence checks in views and fragments, nested elements, empty files, invalid XML, special character IDs, and the validatedIds exclusion list.

  • 🔄 Regenerate and Update Summary
  • ✏️ Insert as PR Description (deletes this comment)
  • 🗑️ Delete comment
PR Bot Information

Version: 1.20.23

  • Summary Prompt: Default Prompt
  • Output Template: Default Template
  • LLM: anthropic--claude-4.6-sonnet
  • File Content Strategy: Full file content
  • Correlation ID: 7f6ca6b8-428f-40f7-98e5-7d6cd51dc3ef
  • Event Trigger: pull_request.opened

Copy link
Copy Markdown
Contributor

@hyperspace-insights hyperspace-insights Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR introduces a useful validateId abstraction in @sap-ux/project-access, but has a critical correctness bug in the async path: findFilesByExtension returns file paths, not file contents, so the XML parser receives path strings instead of XML and ID collision detection never fires when appPath is used. There are also secondary issues: a dead-code assignment in the async IIFE (fileContents is always undefined there), and the synchronous-overload tests are all async/await which silently masks whether the synchronous contract is actually being exercised.

PR Bot Information

Version: 1.20.23

  • LLM: anthropic--claude-4.6-sonnet
  • File Content Strategy: Full file content
  • Agent Instructions:
  • Correlation ID: 7f6ca6b8-428f-40f7-98e5-7d6cd51dc3ef
  • Event Trigger: pull_request.opened

Comment thread packages/project-access/src/library/helpers.ts Outdated
Comment thread packages/project-access/src/library/helpers.ts Outdated
Comment thread packages/fe-fpm-writer/src/common/file.ts Outdated
Comment thread packages/project-access/test/library/helpers.test.ts Outdated
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@815are 815are left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked code:

  1. code changes are easy to understand and approach to use fast-xml-parser instead of xmldom/xmldom
  2. changelog persists
  3. I did not test manually

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants