Refactor file serialization logic in import and export APIs #1050
+886
−542
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughCentralized XML/JSON/YAML (de)serialization by adding FileSerializationUtil, FileSerializationConfig (XmlConfig/JsonConfig/YamlConfig), and FileSerializationException; moved FileContent to a new package and refactored multiple service modules to use the unified API instead of ad‑hoc per‑format logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as ServerService
participant Serializer as FileSerializationUtil
participant ParserLib as "JAXB / Jackson / SnakeYAML"
Client->>Service: request export or upload (file/model)
Service->>Serializer: serialize(entity, fileName, fileType, config) / deserialize(fileContent, targetClass, config)
Serializer->>ParserLib: invoke per-format marshal/unmarshal
ParserLib-->>Serializer: bytes or object
Serializer-->>Service: FileContent or model
Service-->>Client: TransferResource or result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...n/identity/api/server/application/management/v1/core/ServerApplicationManagementService.java
Show resolved
Hide resolved
...mon/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java
Show resolved
Hide resolved
...mon/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java
Show resolved
Hide resolved
....server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/YamlConfig.java
Show resolved
Hide resolved
...rc/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@components/org.wso2.carbon.identity.api.server.application.management/org.wso2.carbon.identity.api.server.application.management.v1/src/main/java/org/wso2/carbon/identity/api/server/application/management/v1/core/ServerApplicationManagementService.java`:
- Around line 626-633: The loop creates a new SnakeYAML TypeDescription for
InboundConfigurationProtocol on each iteration causing earlier protocol mappings
to be overwritten; instead, build a single TypeDescription for
InboundConfigurationProtocol, call description.addPropertyParameters("type",
protocol) for each protocol in INBOUND_CONFIG_PROTOCOLS, then call
constructor.addTypeDescription(description) once (keep references to YamlConfig,
INBOUND_CONFIG_PROTOCOLS, TypeDescription, InboundConfigurationProtocol, and
constructor.addTypeDescription to locate the code).
In
`@components/org.wso2.carbon.identity.api.server.claim.management/org.wso2.carbon.identity.rest.api.server.claim.management.v1/src/main/java/org/wso2/carbon/identity/rest/api/server/claim/management/v1/core/ServerClaimManagementService.java`:
- Around line 751-763: Add explicit allow-list validation for the requested
media type: in exportClaimDialectToFile, check fileType against the supported
media types used by FileSerializationUtil.serialize (reject unsupported values
rather than relying on its fallback to YAML) and throw a ClaimMetadataException
(using the existing error patterns) when the type is not allowed; alternatively,
modify FileSerializationUtil.serialize to detect unsupported media types and
throw FileSerializationException instead of defaulting to YAML so callers like
exportClaimDialectToFile receive a failure and can return an error to the
client.
In
`@components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java`:
- Around line 281-296: The deserializeFromYaml method currently builds a
Constructor and Yaml without applying the configured constructorCustomizer from
FileSerializationConfig/YamlConfig, so custom type loaders/type descriptions are
ignored; update deserializeFromYaml to retrieve the constructorCustomizer (same
way serializeToYaml does), and if present invoke it to customize the Constructor
instance (the one created as new Constructor(targetClass, loaderOptions)) before
creating the Yaml and calling yaml.loadAs; ensure you reference the existing
symbols (deserializeFromYaml, FileSerializationConfig.getYamlConfig(),
constructorCustomizer, Constructor, loaderOptions, yaml.loadAs) so the
customization is applied consistently with serializeToYaml.
🧹 Nitpick comments (2)
components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/JsonConfig.java (1)
35-37: Inconsistent defensive copying in setter.
getSubtypes()defensively clones the array, butsetSubtypes()stores the reference directly. If the caller modifies the passed array after calling the setter, the internal state will change unexpectedly.♻️ Suggested fix
public void setSubtypes(Class<?>... classes) { - this.subtypes = classes; + this.subtypes = classes != null ? classes.clone() : new Class<?>[0]; }components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/XmlConfig.java (1)
40-42: Inconsistent defensive copying in setter.Same issue as in
JsonConfig:getAdditionalJaxbClasses()defensively clones the array, butsetAdditionalJaxbClasses()stores the reference directly.♻️ Suggested fix
public void setAdditionalJaxbClasses(Class<?>... classes) { - this.additionalJaxbClasses = classes; + this.additionalJaxbClasses = classes != null ? classes.clone() : new Class<?>[0]; }
...n/identity/api/server/application/management/v1/core/ServerApplicationManagementService.java
Show resolved
Hide resolved
...2/carbon/identity/rest/api/server/claim/management/v1/core/ServerClaimManagementService.java
Show resolved
Hide resolved
...mon/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors file serialization logic used in import and export APIs by extracting common serialization/deserialization code into reusable utility classes. The refactoring consolidates duplicate XML, JSON, and YAML parsing logic that was previously scattered across multiple service classes.
Changes:
- Moved
FileContentclass to a newfilesubpackage and created centralized serialization utilities - Introduced
FileSerializationUtilwith pluggable configuration for XML, JSON, and YAML formats - Replaced implementation-specific serialization code in UserStore, IDP, Claims, and Application management services with calls to the new utility
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| FileContent.java | Moved from root common package to file subpackage |
| FileSerializationUtil.java | New centralized utility for serializing/deserializing objects to XML/JSON/YAML |
| FileSerializationException.java | New exception class for serialization errors with operation context |
| FileSerializationConfig.java | New configuration holder for format-specific settings |
| YamlConfig.java, XmlConfig.java, JsonConfig.java | New format-specific configuration classes |
| ServerUserStoreService.java | Refactored to use new serialization utility, removed duplicate parsing methods |
| ServerIdpManagementService.java | Refactored to use new serialization utility with custom YAML configuration for property exclusion |
| ServerClaimManagementService.java | Refactored to use new serialization utility with custom YAML dumper options |
| ServerApplicationManagementService.java | Refactored to use new serialization utility with configuration for all formats |
| UserstoresApiServiceImpl.java, IdentityProvidersApiServiceImpl.java, ClaimManagementApiServiceImpl.java | Updated imports for relocated FileContent class |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...n/identity/api/server/application/management/v1/core/ServerApplicationManagementService.java
Show resolved
Hide resolved
...n/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationConfig.java
Show resolved
Hide resolved
…senter to Application serialization
…ation serialization for backward compatibility
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java`:
- Around line 64-83: The serialize method leaves the config null-check empty
causing NPEs when calling config.getXmlConfig()/getJsonConfig()/getYamlConfig();
fix by initializing a default FileSerializationConfig when config is null (e.g.,
if (config == null) { config = new FileSerializationConfig(); }) before
computing mediaType and calling serializeToXml/serializeToJson/serializeToYaml;
keep the rest of the switch and handleUnsupportedSerialize(...) logic unchanged.
♻️ Duplicate comments (1)
components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java (1)
348-368: ApplyYamlConfig.constructorCustomizerduring YAML deserialization.
serializeToYamlapplies the constructor customizer, butdeserializeFromYamlignores it, so custom loaders/type descriptions won’t be honored.🛠️ Proposed fix
- Constructor constructor = new Constructor(targetClass, loaderOptions); + Constructor constructor = new Constructor(targetClass, loaderOptions); + if (config.getYamlConfig().getConstructorCustomizer() != null) { + config.getYamlConfig().getConstructorCustomizer().accept(constructor); + } Yaml yaml = new Yaml(constructor);
...mon/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java`:
- Around line 304-323: The deserializeFromXml method in FileSerializationUtil
currently calls unmarshaller.unmarshal on a StringReader which leaves XML
parsing vulnerable to XXE; fix it by creating a secure StAX XMLInputFactory
(disable DTDs and external entity resolution via IS_SUPPORTING_EXTERNAL_ENTITIES
and SUPPORT_DTD or equivalent parser properties, and set a safe XMLResolver/null
resolver), build an XMLStreamReader from the fileContent string, and pass that
XMLStreamReader to Unmarshaller.unmarshal instead of the StringReader; keep the
JAXBContext/Unmarshaller usage (JAXBContext.newInstance, unmarshaller) and
ensure exceptions are still wrapped in FileSerializationException.
♻️ Duplicate comments (1)
components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java (1)
349-369: Apply constructorCustomizer during YAML deserialization.
deserializeFromYamlbuilds aConstructorbut never appliesYamlConfig#getConstructorCustomizer(), so any custom loaders/type descriptions are ignored. This diverges fromserializeToYamland will surprise callers.🛠️ Proposed fix
Constructor constructor = new Constructor(targetClass, loaderOptions); + if (config.getYamlConfig().getConstructorCustomizer() != null) { + config.getYamlConfig().getConstructorCustomizer().accept(constructor); + } Yaml yaml = new Yaml(constructor);SnakeYAML Constructor customizer apply to loadAs deserialization
...mon/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java
Show resolved
Hide resolved
…teness Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
lashinijay
reviewed
Jan 26, 2026
...mon/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationConfig.java`:
- Around line 84-95: In FileSerializationConfig, add null guards in the setters
setSerializeDefault(DefaultFormat serializeDefault) and
setDeserializeDefault(DefaultFormat deserializeDefault) so they reject null
inputs consistently with the other config checks: validate the parameter is not
null and throw an IllegalArgumentException (with a clear message like
"serializeDefault cannot be null" / "deserializeDefault cannot be null") if it
is, otherwise assign to this.serializeDefault / this.deserializeDefault; this
prevents NPEs later when branching on media type.
In
`@components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java`:
- Around line 346-351: The deserialization path in FileSerializationUtil
registers subtypes without null-safety; update the code that creates
ObjectMapper (in FileSerializationUtil) to guard calls to
config.getJsonConfig().getSubtypes() — first ensure config.getJsonConfig() and
its getSubtypes() return non-null (or default to an empty array) before checking
length or calling objectMapper.registerSubtypes(...); this mirrors the
serialization-side fix and prevents a potential NPE when getSubtypes() is null.
🧹 Nitpick comments (2)
components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationException.java (1)
27-67: Well-structured exception with useful context fields.The exception design provides good debugging context with operation type, file name, and media type. Consider adding a
serialVersionUIDsinceExceptionimplementsSerializable.♻️ Optional: Add serialVersionUID
public class FileSerializationException extends Exception { + private static final long serialVersionUID = 1L; + private final String fileName;components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java (1)
224-225: Consider simplifying string concatenation.
StringBuilderis unnecessary for simple single concatenation operations. Modern Java compilers optimize+concatenation efficiently.♻️ Simplified approach (applies to all three locations)
- StringBuilder fileNameBuilder = new StringBuilder(fileName); - fileNameBuilder.append(Constants.XML_FILE_EXTENSION); + String fullFileName = fileName + Constants.XML_FILE_EXTENSION; ... - return new FileContent(fileNameBuilder.toString(), Constants.MEDIA_TYPE_XML, + return new FileContent(fullFileName, Constants.MEDIA_TYPE_XML,Same pattern applies to JSON (lines 257-258) and YAML (lines 280-281) methods.
...n/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationConfig.java
Outdated
Show resolved
Hide resolved
...mon/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/YamlConfig.java`:
- Around line 84-88: The setAdditionalTrustedClasses method currently iterates
over the varargs and will NPE if callers pass null; update
setAdditionalTrustedClasses to treat a null classes argument as "clear the list"
by assigning additionalTrustedClassNames = new ArrayList<>() and returning early
when classes == null, otherwise populate additionalTrustedClassNames from
classes (use clazz.getName()) as it currently does; reference the method name
setAdditionalTrustedClasses and the field additionalTrustedClassNames when
making the change.
♻️ Duplicate comments (2)
components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationUtil.java (1)
317-333: XML deserialization remains vulnerable to XXE if untrusted input is possible.
Unmarshaller.unmarshal(new StringReader(...))depends on parser defaults; those are not consistently safe across JAXB implementations. Consider a hardened StAX reader (or a feature-flagged secure mode) to mitigate XXE/SSRF risks.🔒 Hardened parsing example (opt-in or guarded)
JAXBContext jaxbContext = JAXBContext.newInstance(classes); Unmarshaller unmarshaller = jaxbContext.createUnmarshaller(); - `@SuppressWarnings`("unchecked") - T result = (T) unmarshaller.unmarshal(new StringReader(fileContent.getContent())); - return result; - } catch (JAXBException e) { + javax.xml.stream.XMLInputFactory xif = javax.xml.stream.XMLInputFactory.newFactory(); + xif.setProperty(javax.xml.stream.XMLInputFactory.SUPPORT_DTD, false); + xif.setProperty(javax.xml.stream.XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); + javax.xml.stream.XMLStreamReader xsr = + xif.createXMLStreamReader(new StringReader(fileContent.getContent())); + `@SuppressWarnings`("unchecked") + T result = (T) unmarshaller.unmarshal(xsr, targetClass).getValue(); + return result; + } catch (JAXBException | javax.xml.stream.XMLStreamException e) { throw new FileSerializationException("Failed to deserialize from XML", e, fileContent.getFileName(), Constants.MEDIA_TYPE_XML, FileSerializationException.Operation.DESERIALIZE); }components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/FileSerializationConfig.java (1)
84-95: Reject null defaults to avoid NPEs in format switching.If a caller sets either default to
null, the switch inhandleUnsupported*will NPE.✅ Suggested guard
public void setSerializeDefault(DefaultFormat serializeDefault) { - this.serializeDefault = serializeDefault; + if (serializeDefault == null) { + throw new IllegalArgumentException("serializeDefault cannot be null"); + } + this.serializeDefault = serializeDefault; } public void setDeserializeDefault(DefaultFormat deserializeDefault) { - this.deserializeDefault = deserializeDefault; + if (deserializeDefault == null) { + throw new IllegalArgumentException("deserializeDefault cannot be null"); + } + this.deserializeDefault = deserializeDefault; }
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/JsonConfig.java (1)
35-37: Defensively copyclassesinsetSubtypesto avoid external mutation.Right now the caller can mutate the varargs array after the setter call. Cloning keeps the config immutable from outside.
♻️ Suggested change
public void setSubtypes(Class<?>... classes) { - this.subtypes = classes; + this.subtypes = classes != null ? classes.clone() : new Class<?>[0]; }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/YamlConfig.java`:
- Around line 84-91: The setAdditionalTrustedClasses method currently assumes
all entries in the varargs are non-null and calls clazz.getName(), which will
throw a NullPointerException for null elements; update
setAdditionalTrustedClasses in YamlConfig to skip null entries (or validate and
throw a clearer IllegalArgumentException) by checking each clazz for null before
calling getName() and only adding non-null clazz.getName() to
additionalTrustedClassNames; ensure additionalTrustedClassNames is still
initialized to an empty list when classes is null or contains only nulls.
....server.common/src/main/java/org/wso2/carbon/identity/api/server/common/file/YamlConfig.java
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Eliminates code duplication across 4 entity modules (Applications, Identity Providers, User Stores, Claims) where each implements nearly identical file serialization/deserialization logic for XML/JSON/YAML formats, creating maintenance burden and inconsistency.
Related to https://github.com/wso2-enterprise/iam-product-management/issues/662
Goals
Introduce a centralized file serialization utility framework that consolidates duplicated code while preserving entity-specific customizations and maintaining backward compatibility with existing import/export functionality.
Approach
Created FileSerializationUtil as the single source of truth for all serialization/deserialization operations with the following architecture:
Entity services now construct configuration objects and delegate to the utility, removing implementation in service sides while preserving exact output formats.
For detailed architecture and migration details please refer:
https://docs.google.com/document/d/1A8gC1wPyTrMIxChN5oBbADigKl9O7zTaGBJ5cBHOQ84/edit?usp=sharing
User stories
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.