Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a centralized date-parsing utility, a new stamp-transaction domain and service, employee/employer subservices, extensive payroll and invoice model expansions (BigDecimal migration and new POJOs), consolidates date handling across download/xml models, adds documentation, and bumps the project version. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant ClientLib as FiscalApiClient
participant StampSvc as StampService
participant HttpClient as IFiscalApiHttpClient
participant API as RemoteAPI
Client->>ClientLib: getStampService().transferStamps(params)
ClientLib->>StampSvc: transferStamps(params)
StampSvc->>HttpClient: POST /api/.../stamps (params)
HttpClient->>API: outbound HTTP request
API-->>HttpClient: HTTP response (ApiResponse<Boolean>)
HttpClient-->>StampSvc: ApiResponse<Boolean>
StampSvc-->>ClientLib: ApiResponse<Boolean>
ClientLib-->>Client: ApiResponse<Boolean>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 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)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds payroll (“nómina”) and local taxes complements plus stamp (timbres) transaction support to the Java SDK, while also refactoring several DTOs for BigDecimal/date parsing and extending the client/service surface area.
Changes:
- Introduces payroll/local-taxes complement DTOs and wires them into
Invoice/issuer/recipient models. - Adds stamp transactions models +
StampServiceand exposes it fromFiscalApiClient. - Refactors multiple money/date fields (BigDecimal + shared date parsing helper) and updates examples accordingly.
Reviewed changes
Copilot reviewed 46 out of 47 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/fiscalapi/services/StampService.java | New service for stamps resource, adds transfer/withdraw operations. |
| src/main/java/com/fiscalapi/services/PersonService.java | Exposes employer/employee sub-services off PersonService. |
| src/main/java/com/fiscalapi/services/InvoiceService.java | Simplifies invoice creation endpoint usage. |
| src/main/java/com/fiscalapi/services/FiscalApiClient.java | Registers/exposes new IStampService from the main client. |
| src/main/java/com/fiscalapi/services/EmployerService.java | Adds employer CRUD-like operations under people. |
| src/main/java/com/fiscalapi/services/EmployeeService.java | Adds employee CRUD-like operations under people. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollSeverance.java | New payroll severance DTO. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollRetirement.java | New payroll retirement DTO. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollEarnings.java | Aggregates payroll earnings/other payments/retirement/severance. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollEarningOvertime.java | New overtime DTO for payroll earning lines. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollEarningOtherPayment.java | New “other payment” DTO (incl. balance compensation). |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollEarning.java | New payroll earning line DTO (taxed/exempt + overtime). |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollDisability.java | New payroll disability DTO. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollDeduction.java | New payroll deduction DTO. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/Payroll.java | New payroll complement root DTO with SAT date formatting. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/EmployerData.java | Employer payroll data attached to issuer. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/EmployeeData.java | Employee payroll data attached to recipient. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/BalanceCompensation.java | New balance compensation DTO used by other payments. |
| src/main/java/com/fiscalapi/models/invoicing/paymentComplement/PaidInvoiceTax.java | Moves to paymentComplement package + BigDecimal base. |
| src/main/java/com/fiscalapi/models/invoicing/paymentComplement/PaidInvoice.java | Moves to paymentComplement package + BigDecimal amounts. |
| src/main/java/com/fiscalapi/models/invoicing/paymentComplement/InvoicePayment.java | Moves to paymentComplement package + BigDecimal amounts, shared date parsing. |
| src/main/java/com/fiscalapi/models/invoicing/localTaxes/LocalTaxes.java | New local taxes complement container. |
| src/main/java/com/fiscalapi/models/invoicing/localTaxes/LocalTax.java | New local tax line DTO. |
| src/main/java/com/fiscalapi/models/invoicing/OnBehalfOf.java | New “on behalf of” DTO for invoice items. |
| src/main/java/com/fiscalapi/models/invoicing/InvoiceRecipient.java | Adds employeeData support. |
| src/main/java/com/fiscalapi/models/invoicing/InvoiceItem.java | Adds unitOfMeasurement/onBehalfOf + BigDecimal quantities/prices. |
| src/main/java/com/fiscalapi/models/invoicing/InvoiceIssuer.java | Adds employerData support. |
| src/main/java/com/fiscalapi/models/invoicing/Invoice.java | Adds complement/paymentConditions + BigDecimal monetary fields + shared date parsing. |
| src/main/java/com/fiscalapi/models/invoicing/Complement.java | New complement wrapper (payroll/payment/local taxes). |
| src/main/java/com/fiscalapi/models/downloading/Xml.java | Refactors SAT date parsing to shared util. |
| src/main/java/com/fiscalapi/models/downloading/MetadataItem.java | Refactors SAT date parsing to shared util. |
| src/main/java/com/fiscalapi/models/downloading/DownloadRequest.java | Adds String-based date setters + LocalDateTime alternatives; uses shared util. |
| src/main/java/com/fiscalapi/models/UserLookupDto.java | New lightweight user reference DTO (for stamp transactions). |
| src/main/java/com/fiscalapi/models/StampTransactionType.java | New enum for stamp transaction types. |
| src/main/java/com/fiscalapi/models/StampTransactionStatus.java | New enum for stamp transaction statuses. |
| src/main/java/com/fiscalapi/models/StampTransactionParams.java | New params DTO for stamp transfer/withdraw calls. |
| src/main/java/com/fiscalapi/models/StampTransaction.java | New stamp transaction DTO. |
| src/main/java/com/fiscalapi/models/ProductTax.java | Changes tax rate type to BigDecimal. |
| src/main/java/com/fiscalapi/models/Product.java | Changes unitPrice type to BigDecimal. |
| src/main/java/com/fiscalapi/models/Person.java | Changes balances to BigDecimal. |
| src/main/java/com/fiscalapi/abstractions/IStampService.java | New service interface for stamp operations. |
| src/main/java/com/fiscalapi/abstractions/IPersonService.java | Adds employer/employee service accessors. |
| src/main/java/com/fiscalapi/abstractions/IFiscalApiClient.java | Adds getStampService(). |
| src/main/java/com/fiscalapi/OptUtil.java | New shared SAT date parsing helper. |
| src/main/java/com/fiscalapi/Examples.java | Adds extensive payroll/local taxes/stamps usage examples. |
| pom.xml | Version bump. |
| CLAUDE.md | Adds repository guidance doc for Claude Code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private String fromPersonId; | ||
| private String toPersonId; | ||
| private int amount; | ||
| public String comments; |
There was a problem hiding this comment.
StampTransactionParams exposes comments as a public field while also providing getters/setters. This breaks encapsulation and makes the DTO inconsistent with others; make it private like the other fields and rely on the accessor methods (or annotate explicitly if needed for JSON).
| public String comments; | |
| private String comments; |
| IFiscalApiHttpClient httpClient; | ||
| private final String baseEndpoint = "api/v4/people"; | ||
|
|
There was a problem hiding this comment.
EmployeeService builds endpoints like "api/v4/people/..." without the base URL and hardcodes the API version. Since FiscalApiHttpClient requires absolute URLs, these calls will fail. Use FiscalApiSettings.getApiUrl()/getApiVersion() (or extend BaseFiscalApiService) to build full URLs consistently.
| import com.fiscalapi.services.EmployeeService; | ||
| import com.fiscalapi.services.EmployerService; | ||
|
|
||
| /** | ||
| * Define el contrato específico para operaciones con "Person". | ||
| * Hereda las operaciones básicas (CRUD) de IFiscalApiService<Person>. | ||
| */ | ||
| public interface IPersonService extends IFiscalApiService<Person> { | ||
| EmployerService employer = null; | ||
|
|
||
| // other specific methods | ||
| EmployeeService getEmployeeService(); | ||
| EmployerService getEmployerService(); |
There was a problem hiding this comment.
IPersonService now imports and exposes concrete service implementations (EmployeeService/EmployerService) and also declares an interface field EmployerService employer = null;. The field is a pointless public static final constant and the concrete-type exposure breaks the abstractions/ layering pattern used by other service interfaces (e.g., IInvoiceService only references models). Prefer returning abstraction interfaces (e.g., IEmployerService/IEmployeeService) or keep these accessors on the concrete PersonService, and remove the interface field.
| @@ -27,17 +30,17 @@ public String getItemCode() { | |||
| public void setItemCode(String itemCode) { | |||
| this.itemCode = itemCode; | |||
| } | |||
| public Double getQuantity() { | |||
| public BigDecimal getQuantity() { | |||
| return quantity; | |||
| } | |||
| public void setQuantity(Double quantity) { | |||
| this.quantity = quantity; | |||
| public void setQuantity(String quantity) { | |||
| this.quantity = new BigDecimal(quantity); | |||
| } | |||
| public Double getDiscount() { | |||
| public BigDecimal getDiscount() { | |||
| return discount; | |||
| } | |||
| public void setDiscount(Double discount) { | |||
| this.discount = discount; | |||
| public void setDiscount(String discount) { | |||
| this.discount = new BigDecimal(discount); | |||
| } | |||
| public String getUnitOfMeasurementCode() { | |||
| return unitOfMeasurementCode; | |||
| @@ -51,11 +54,11 @@ public String getDescription() { | |||
| public void setDescription(String description) { | |||
| this.description = description; | |||
| } | |||
| public Double getUnitPrice() { | |||
| public BigDecimal getUnitPrice() { | |||
| return unitPrice; | |||
| } | |||
| public void setUnitPrice(Double unitPrice) { | |||
| this.unitPrice = unitPrice; | |||
| public void setUnitPrice(String unitPrice) { | |||
| this.unitPrice = new BigDecimal(unitPrice); | |||
There was a problem hiding this comment.
InvoiceItem quantity/discount/unitPrice setters were changed to accept String and parse BigDecimal. This is a breaking API change and will throw on null/empty inputs. Provide BigDecimal (and/or numeric) overloads to preserve SDK ergonomics, and if this is intended for JSON binding prefer Jackson configuration/annotations rather than changing the public setter signatures.
| @Override | ||
| public ApiResponse<Boolean> TransferStamps(StampTransactionParams requestModel) { | ||
| return httpClient.post(buildEndpoint("", null), requestModel, Boolean.class); | ||
| } | ||
|
|
||
| @Override | ||
| public ApiResponse<Boolean> WithdrawStamps(StampTransactionParams requestModel) { | ||
| return httpClient.post(buildEndpoint("", null), requestModel, Boolean.class); | ||
| } |
There was a problem hiding this comment.
TransferStamps and WithdrawStamps are currently identical (same endpoint and HTTP method). If the API distinguishes transfer vs withdraw, this will call the wrong operation; if it doesn’t, having two methods is misleading. Use distinct action paths (e.g., "transfer" vs "withdraw") or consolidate into a single method with an explicit operation parameter.
| public interface IStampService extends IFiscalApiService<StampTransaction>{ | ||
| ApiResponse<Boolean> TransferStamps(StampTransactionParams requestModel); | ||
| ApiResponse<Boolean> WithdrawStamps(StampTransactionParams requestModel); | ||
| } |
There was a problem hiding this comment.
Method names in this public interface use PascalCase (TransferStamps/WithdrawStamps), which is inconsistent with the rest of the SDK APIs (e.g., getById/create/update/delete). This is a breaking/awkward public surface; rename to lowerCamelCase (transferStamps/withdrawStamps) and keep backwards compatibility via default methods or deprecated aliases if needed.
| public void seTransactionStatus(StampTransactionStatus transactionStatus) { | ||
| this.transactionStatus = transactionStatus; | ||
| } |
There was a problem hiding this comment.
StampTransaction has a typo in the setter name: seTransactionStatus is missing the 't' (should be setTransactionStatus). This breaks JavaBeans conventions and will prevent typical frameworks/Jackson from setting transactionStatus via the setter.
| public void seTransactionStatus(StampTransactionStatus transactionStatus) { | |
| this.transactionStatus = transactionStatus; | |
| } | |
| public void setTransactionStatus(StampTransactionStatus transactionStatus) { | |
| this.transactionStatus = transactionStatus; | |
| } | |
| @Deprecated | |
| public void seTransactionStatus(StampTransactionStatus transactionStatus) { | |
| setTransactionStatus(transactionStatus); | |
| } |
| return employerService; | ||
| } | ||
|
|
||
| public EmployeeService getEmployeeService() { |
There was a problem hiding this comment.
This method overrides IPersonService.getEmployeeService; it is advisable to add an Override annotation.
| */ | ||
| public class PersonService extends BaseFiscalApiService<Person> implements IPersonService { | ||
|
|
||
| public EmployerService getEmployerService() { |
There was a problem hiding this comment.
This method overrides IPersonService.getEmployerService; it is advisable to add an Override annotation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/fiscalapi/models/invoicing/paymentComplement/PaidInvoiceTax.java (1)
34-36:⚠️ Potential issue | 🟡 MinorSame exception risk in
setTaxRate.Apply the same fix as suggested for
setBaseto handle null/malformed input consistently.
🤖 Fix all issues with AI agents
In `@src/main/java/com/fiscalapi/models/downloading/DownloadRequest.java`:
- Line 7: DownloadRequest's String-based setters (setStartDate, setEndDate,
setLastAttemptDate, setNextAttemptDate) call OptUtil.formatInputDateToSATFormat
and end up storing LocalDateTime.MIN for null/blank input; guard the input by
checking if the incoming String is null or blank and set the corresponding field
to null in that case, otherwise call OptUtil.formatInputDateToSATFormat and
assign the parsed value; update the logic inside the four methods
(setStartDate(String), setEndDate(String), setLastAttemptDate(String),
setNextAttemptDate(String)) to perform this null/blank check before parsing so
SAT-invalid -999999999 dates are not produced.
In `@src/main/java/com/fiscalapi/models/downloading/MetadataItem.java`:
- Around line 155-158: setSatInvoiceDate currently assigns the result of
OptUtil.formatInputDateToSATFormat(satDate) directly, but OptUtil returns
LocalDateTime.MIN for null/empty inputs which then gets serialized as a bogus
date; change setSatInvoiceDate to short‑circuit null/empty inputs or check the
returned LocalDateTime and set this.invoiceDate to null when the input is
null/empty or when the formatted value equals LocalDateTime.MIN, otherwise
assign the formatted value; apply the same pattern to the other MetadataItem
setters that call OptUtil.formatInputDateToSATFormat so any SAT date formatter
result equal to LocalDateTime.MIN is treated as unset (null).
In `@src/main/java/com/fiscalapi/models/invoicing/Invoice.java`:
- Line 8: In class Invoice, the setters setSubtotal, setDiscount, setTotal, and
setExchangeRate should guard against null or blank string input before calling
new BigDecimal(String); update each setter to check if the input string is null
or blank (e.g., null or trim().isEmpty()) and if so set the corresponding
BigDecimal field to null (or leave it unset), otherwise construct new BigDecimal
from the trimmed string and assign it—apply this exact pattern to setSubtotal,
setDiscount, setTotal, and setExchangeRate to avoid
NullPointerException/NumberFormatException during deserialization.
- Around line 216-226: Remove the Jackson setter conflict by annotating the
plain JavaBean setter setDate(String) with `@JsonIgnore` so only
setSatDate(String) (annotated with `@JsonProperty`("date")) is used for JSON
deserialization; additionally, inside setDate(String) and setSatDate(String) add
null/empty guards before calling
com.fiscalapi.OptUtil.formatInputDateToSATFormat(...) and only assign to
this.date when the input is non-null/non-empty (to avoid
formatInputDateToSATFormat returning LocalDateTime.MIN or otherwise unintended
values).
In `@src/main/java/com/fiscalapi/models/invoicing/InvoiceItem.java`:
- Line 3: In InvoiceItem, the String-based setters setQuantity, setDiscount, and
setUnitPrice must guard against null/blank strings before calling new
BigDecimal(...) to avoid NPE/NumberFormatException during deserialization;
update each setter to check if the input is null or input.trim().isEmpty() and
if so assign the corresponding BigDecimal field to null (or a safe default),
otherwise call new BigDecimal(input.trim()) (optionally wrap the construction in
a try/catch to rethrow a clear IllegalArgumentException on invalid number
formats).
In `@src/main/java/com/fiscalapi/models/invoicing/localTaxes/LocalTax.java`:
- Around line 23-25: The setters in LocalTax (setTaxRate and setTaxAmount) take
String and directly new BigDecimal(...) which throws NPE/NumberFormatException;
update these methods in class LocalTax to either accept BigDecimal parameters
(preferred for consistency with PayrollSeverance) or add defensive handling:
check for null/blank, validate/parse safely (e.g., catch NumberFormatException)
and set fields taxRate/taxAmount to null or a default BigDecimal.ZERO as
appropriate, and document behavior; ensure the methods preserve the BigDecimal
field types and mirror the null-safe pattern used in
PayrollDeduction/PayrollSeverance so callers and serialization remain
consistent.
In
`@src/main/java/com/fiscalapi/models/invoicing/paymentComplement/InvoicePayment.java`:
- Around line 69-79: The InvoicePayment class has two setters that map to the
same JSON property—setSatDate(String) is annotated with
`@JsonProperty`("paymentDate") while setPaymentDate(String) is also picked up by
Jackson via naming convention—causing a conflicting-setter error; fix this by
annotating the plain String overload setPaymentDate(String) with `@JsonIgnore` so
Jackson only uses setSatDate(String) for deserialization, leaving the formatting
call to com.fiscalapi.OptUtil.formatInputDateToSATFormat intact.
In
`@src/main/java/com/fiscalapi/models/invoicing/paymentComplement/PaidInvoice.java`:
- Around line 1-18: The BigDecimal setters in PaidInvoice (setPaymentAmount,
setSubTotal, setPreviousBalance, setRemainingBalance, setEquivalence) call new
BigDecimal(...) directly on String inputs and throw NumberFormatException for
null/blank values; update each setter to first check if the incoming String is
null or blank (use StringUtils.isBlank or trim().isEmpty()) and if so set the
corresponding field to null (or leave it unchanged), otherwise construct new
BigDecimal from the trimmed string—ensuring you reference the paymentAmount,
subTotal, previousBalance, remainingBalance, and equivalence fields and their
respective setter methods when making the change.
In
`@src/main/java/com/fiscalapi/models/invoicing/paymentComplement/PaidInvoiceTax.java`:
- Around line 16-18: The setBase(String base) setter in PaidInvoiceTax currently
calls new BigDecimal(base) which can throw NumberFormatException; either change
the setter signature to setBase(BigDecimal base) (updating all callers) to match
other models like ProductTax/Product, or if you must keep a String overload, add
null and format validation inside setBase(String) (check for null/empty, catch
NumberFormatException) and convert safely to this.base (or delegate to
setBase(BigDecimal)) while preserving existing behavior and consistency with the
class field named base.
In `@src/main/java/com/fiscalapi/models/invoicing/payroll/EmployeeData.java`:
- Around line 84-95: The setter for laborRelationStartDate should avoid storing
the sentinel LocalDateTime.MIN when input is null/blank: in
setLaborRelationStartDate(String) (EmployeeData) check for null or blank input
and set this.laborRelationStartDate = null; otherwise call
OptUtil.formatInputDateToSATFormat and assign; also harden
getLaborRelationStartDate() to treat LocalDateTime.MIN as missing by returning
null if laborRelationStartDate == null ||
laborRelationStartDate.equals(LocalDateTime.MIN) before formatting with
SAT_DATE_FORMAT_OUT.
- Around line 261-270: The setters setBaseSalaryForContributions and
setIntegratedDailySalary should guard against null or blank input before calling
new BigDecimal(...); update each setter to check if the incoming String is null
or blank (e.g., trim and isEmpty/isBlank) and assign the corresponding
BigDecimal field to null in that case, otherwise construct new BigDecimal from
the validated string so Jackson deserialization won't throw
NullPointerException/NumberFormatException.
In `@src/main/java/com/fiscalapi/models/invoicing/payroll/Payroll.java`:
- Around line 38-71: The setters setPaymentDate, setInitialPaymentDate, and
setFinalPaymentDate are assigning LocalDateTime.MIN when
OptUtil.formatInputDateToSATFormat receives null/blank, causing invalid
serialization; change each setter to first check if the input string is null or
blank and set the corresponding field to null in that case, otherwise call
com.fiscalapi.OptUtil.formatInputDateToSATFormat(paymentDate) (or the matching
param) and assign the result; optionally note that a better centralized fix is
to have OptUtil.formatInputDateToSATFormat return null instead of
LocalDateTime.MIN so callers like these setters don't need repeated guards.
In `@src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollDeduction.java`:
- Around line 39-41: The setter setAmount in PayrollDeduction currently accepts
a String and constructs new BigDecimal(amount), which breaks JavaBean symmetry
with the BigDecimal-typed field and may throw NPE/NumberFormatException; change
setAmount to accept a BigDecimal parameter (matching PayrollSeverance) or, if
String input is required, add null-check and validation/parsing with clear error
handling (e.g., null => set to null or throw IllegalArgumentException; catch
NumberFormatException and rethrow with contextual message) so that
PayrollDeduction.setAmount and its BigDecimal field/ getter are consistent and
safe.
In `@src/main/java/com/fiscalapi/models/StampTransaction.java`:
- Around line 75-77: The setter method in StampTransaction is misspelled as
seTransactionStatus; rename this method to setTransactionStatus to follow
JavaBean conventions so frameworks can map the transactionStatus field
correctly, update any references/callers/tests to the new method name, and
ensure the method still assigns this.transactionStatus = transactionStatus in
the StampTransaction class.
In `@src/main/java/com/fiscalapi/models/StampTransactionParams.java`:
- Line 8: The field "comments" in class StampTransactionParams is declared
public, breaking encapsulation and differing from other private fields; change
the declaration of the comments field to private and add standard accessor
methods getComments() and setComments(String comments) in StampTransactionParams
so it matches the existing getter/setter pattern used by other fields and
preserves API/serialization expectations.
In `@src/main/java/com/fiscalapi/OptUtil.java`:
- Around line 10-14: In OptUtil.formatInputDateToSATFormat change the sentinel
return value: instead of initializing and returning LocalDateTime.MIN when
stringDate is null or empty, return null so callers can detect missing dates;
update the method's local variable (e.g., parsedDate) to allow null and ensure
callers like Xml.getSatDate() handle a null return appropriately.
In `@src/main/java/com/fiscalapi/services/EmployeeService.java`:
- Around line 26-36: Add null-checks for requestModel and for
requestModel.getEmployeePersonId() at the start of both Create and Update
methods and throw an IllegalArgumentException with a clear message if either is
null; specifically validate inside Create(...) and Update(...) before calling
buildEndpoint(requestModel.getEmployeePersonId()), e.g., ensure requestModel !=
null and requestModel.getEmployeePersonId() != null and throw
IllegalArgumentException("...") when validation fails so buildEndpoint and
downstream httpClient calls never receive "null" as the id.
In `@src/main/java/com/fiscalapi/services/InvoiceService.java`:
- Around line 60-63: The create method in InvoiceService currently always posts
to the base invoices endpoint; restore type-based routing by inspecting
model.getTypeCode() inside create and map "I" -> "income", "E" -> "credit-note",
"P" -> "payment", then call buildEndpoint(mappedPath, null) and pass that
endpoint to httpClient.post(model, Invoice.class); for any other type throw an
IllegalArgumentException("Invalid invoice type") so credit notes and payments
route to their correct endpoints.
In `@src/main/java/com/fiscalapi/services/StampService.java`:
- Around line 21-29: TransferStamps and WithdrawStamps both post to
buildEndpoint("", null) with identical StampTransactionParams so the backend
cannot distinguish actions; update these methods to call distinct endpoints
(e.g., use buildEndpoint("transfer", null) in TransferStamps and
buildEndpoint("withdraw", null) in WithdrawStamps) or alternately add a
transactionType/action field to StampTransactionParams and set it appropriately
before posting, ensuring the chosen approach is implemented consistently in the
TransferStamps and WithdrawStamps methods and any consumers of
StampTransactionParams.
🧹 Nitpick comments (20)
src/main/java/com/fiscalapi/models/Person.java (1)
26-27: Consider importingBigDecimalfor consistency.The file uses import statements for other types (e.g.,
LocalDateTime), butBigDecimalis referenced with its fully qualified name throughout. Adding an import would improve readability and align with the existing code style.✨ Proposed refactor to use import
Add the import at the top of the file:
import java.math.BigDecimal;Then update the field declarations and methods:
- private java.math.BigDecimal availableBalance; - private java.math.BigDecimal committedBalance; + private BigDecimal availableBalance; + private BigDecimal committedBalance;- public java.math.BigDecimal getAvailableBalance() { + public BigDecimal getAvailableBalance() { return availableBalance; } - public void setAvailableBalance(java.math.BigDecimal availableBalance) { + public void setAvailableBalance(BigDecimal availableBalance) { this.availableBalance = availableBalance; } - public java.math.BigDecimal getCommittedBalance() { + public BigDecimal getCommittedBalance() { return committedBalance; } - public void setCommittedBalance(java.math.BigDecimal committedBalance) { + public void setCommittedBalance(BigDecimal committedBalance) { this.committedBalance = committedBalance; }Also applies to: 149-149, 153-153, 157-157, 161-161
src/main/java/com/fiscalapi/models/UserLookupDto.java (1)
5-25: Field declaration should be grouped at the top.The
legalNamefield is declared at line 24, after its getter/setter methods. For consistency and readability, declare all fields together at the top of the class.♻️ Suggested reordering
public class UserLookupDto extends BaseDto { private String tin; + private String legalName; public String getLegalName() { return legalName; } public void setLegalName(String legalName) { this.legalName = legalName; } public String getTin() { return tin; } public void setTin(String tin) { this.tin = tin; } - - private String legalName; }src/main/java/com/fiscalapi/abstractions/IStampService.java (1)
7-10: Method names should use camelCase instead of PascalCase.
TransferStampsandWithdrawStampsshould betransferStampsandwithdrawStampsto follow Java naming conventions and maintain consistency with all other service interfaces in the codebase.♻️ Proposed fix
public interface IStampService extends IFiscalApiService<StampTransaction>{ - ApiResponse<Boolean> TransferStamps(StampTransactionParams requestModel); - ApiResponse<Boolean> WithdrawStamps(StampTransactionParams requestModel); + ApiResponse<Boolean> transferStamps(StampTransactionParams requestModel); + ApiResponse<Boolean> withdrawStamps(StampTransactionParams requestModel); }src/main/java/com/fiscalapi/models/ProductTax.java (1)
34-34: Use the importedBigDecimalclass name for consistency.The fully qualified
java.math.BigDecimalis used here and in the getter/setter (lines 96, 105), but the import already exists at line 5. Consider using the short formBigDecimalfor cleaner code.♻️ Proposed fix
- private java.math.BigDecimal rate; + private BigDecimal rate;And similarly for the getter/setter signatures.
CLAUDE.md (2)
24-32: Add language specifier to fenced code block.Static analysis flagged this code block as missing a language specifier. Since this is a text-based diagram, use
textorplaintext.📝 Proposed fix
-``` +```text IFiscalApiClient (facade) ├── getInvoiceService() → IInvoiceService extends IFiscalApiService<Invoice>
45-47: Add language specifier to fenced code block.Same issue as above - add
textlanguage specifier for the DTO hierarchy diagram.📝 Proposed fix
-``` +```text SerializableDto → AuditableDto (createdAt, updatedAt) → BaseDto (id)src/main/java/com/fiscalapi/models/invoicing/payroll/BalanceCompensation.java (1)
6-9: Consider usingIntegerfor nullable year representation.Using primitive
intforyearmeans it defaults to 0 when not set. In a payroll context, distinguishing between "year not provided" and "year 0" (or any valid year) may be important for validation and serialization.♻️ Proposed change
public class BalanceCompensation extends CatalogDto { private BigDecimal favorableBalance; - private int year; + private Integer year; private BigDecimal remainingFavorableBalance;Update getter/setter accordingly:
- public int getYear() { + public Integer getYear() { return year; } - public void setYear(int year) { + public void setYear(Integer year) { this.year = year; }src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollDisability.java (1)
6-31: Use lowerCamelCase for private fields.
Capitalized field names are non-idiomatic in Java and make the DTO harder to read. Rename the backing fields and update assignments to match.♻️ Proposed refactor
- private int DisabilityDays; - private String DisabilityTypeCode; - private BigDecimal MonetaryAmount; + private int disabilityDays; + private String disabilityTypeCode; + private BigDecimal monetaryAmount; public int getDisabilityDays() { - return DisabilityDays; + return disabilityDays; } public void setDisabilityDays(int disabilityDays) { - DisabilityDays = disabilityDays; + this.disabilityDays = disabilityDays; } public String getDisabilityTypeCode() { - return DisabilityTypeCode; + return disabilityTypeCode; } public void setDisabilityTypeCode(String disabilityTypeCode) { - DisabilityTypeCode = disabilityTypeCode; + this.disabilityTypeCode = disabilityTypeCode; } public BigDecimal getMonetaryAmount() { - return MonetaryAmount; + return monetaryAmount; } public void setMonetaryAmount(BigDecimal monetaryAmount) { - MonetaryAmount = monetaryAmount; + this.monetaryAmount = monetaryAmount; }src/main/java/com/fiscalapi/services/EmployerService.java (1)
20-41: Deduplicate endpoint construction.
This mirrors EmployeeService and avoids repeated String.format blocks across methods.♻️ Proposed refactor
public EmployerService(IFiscalApiHttpClient httpClient) { this.httpClient = httpClient; } + private String buildEndpoint(String personId) { + return String.format("%s/%s/employer", baseEndpoint, personId); + } + public ApiResponse<EmployerData> GetById(String id) { - String endpoint = String.format("%s/%s/employer", baseEndpoint, id); - - return httpClient.get(endpoint, EmployerData.class); + return httpClient.get(buildEndpoint(id), EmployerData.class); } public ApiResponse<EmployerData>Create(EmployerData requestModel) { - String endpoint = String.format("%s/%s/employer", baseEndpoint, requestModel.getPersonId()); - - return httpClient.post(endpoint, requestModel, EmployerData.class); + return httpClient.post(buildEndpoint(requestModel.getPersonId()), requestModel, EmployerData.class); } public ApiResponse<EmployerData> Update(EmployerData requestModel) { - String endpoint = String.format("%s/%s/employer", baseEndpoint, requestModel.getPersonId()); - - return httpClient.put(endpoint, requestModel, EmployerData.class); + return httpClient.put(buildEndpoint(requestModel.getPersonId()), requestModel, EmployerData.class); } public ApiResponse<Boolean> Delete(String id) { - String endpoint = String.format("%s/%s/employer", baseEndpoint, id); - - return httpClient.delete(endpoint); + return httpClient.delete(buildEndpoint(id)); }src/main/java/com/fiscalapi/abstractions/IPersonService.java (1)
12-12: Remove the unused interface constant.
Interface fields are public static final; keeping a null constant here is misleading.♻️ Proposed refactor
- EmployerService employer = null;src/main/java/com/fiscalapi/models/downloading/Xml.java (2)
9-11: Unused imports after refactoring.
ZonedDateTimeandDateTimeParseExceptionare no longer used in this file since date parsing logic has been moved toOptUtil. Consider removing these unused imports.♻️ Proposed fix
import java.math.BigDecimal; import java.time.LocalDateTime; -import java.time.ZonedDateTime; -import java.time.format.DateTimeParseException; import java.util.ArrayList;
178-187:setDate(String)duplicatessetSatDate(String)functionality.Both methods do exactly the same thing—parse a string date using
OptUtil.formatInputDateToSATFormatand assign tothis.date. The only difference issetSatDateis annotated with@JsonProperty("date")for JSON deserialization.If
setDate(String)is intended as a programmatic convenience setter, this is acceptable but consider documenting the distinction. Otherwise, this introduces API surface redundancy.src/main/java/com/fiscalapi/models/invoicing/InvoiceRecipient.java (1)
12-22: Field declaration should precede its accessors.The
employeeDatafield is declared at line 21, after its getter and setter (lines 13-19). This breaks Java convention where fields are typically declared at the top of the class before methods. The same pattern is followed elsewhere in this file (e.g.,id,tin, etc. are declared before their accessors).♻️ Proposed fix: move field before accessors
private String cfdiUseCode; + private EmployeeData employeeData; public EmployeeData getEmployeeData() { return employeeData; } public void setEmployeeData(EmployeeData employeeData) { this.employeeData = employeeData; } - private EmployeeData employeeData; private String zipCode;src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollEarningOvertime.java (1)
32-41: Field placement and import style inconsistencies.Two issues:
- The
amountPaidfield (line 40) is declared after its getter/setter, breaking Java convention.java.math.BigDecimalis used as a fully qualified name instead of an import statement. Other payroll models use an import at the top.♻️ Proposed fix
package com.fiscalapi.models.invoicing.payroll; +import java.math.BigDecimal; + public class PayrollEarningOvertime { private int days; private String hoursTypeCode; private int extraHours; + private BigDecimal amountPaid; public int getDays() { return days; } @@ -32,11 +34,9 @@ this.extraHours = extraHours; } - public java.math.BigDecimal getAmountPaid() { + public BigDecimal getAmountPaid() { return amountPaid; } - public void setAmountPaid(java.math.BigDecimal amountPaid) { + public void setAmountPaid(BigDecimal amountPaid) { this.amountPaid = amountPaid; } - - private java.math.BigDecimal amountPaid; }src/main/java/com/fiscalapi/OptUtil.java (1)
10-10: Method nameformatInputDateToSATFormatis misleading.The method parses a date string into a
LocalDateTimeobject—it doesn't format anything. A name likeparseDateFromSATFormatorparseToLocalDateTimewould better describe the behavior.src/main/java/com/fiscalapi/services/PersonService.java (1)
14-23: Field declarations should precede their accessor methods.Fields
employerServiceandemployeeService(lines 22-23) are declared after their getters (lines 14-20). Move field declarations to precede the methods for consistency with Java conventions.♻️ Proposed fix
public class PersonService extends BaseFiscalApiService<Person> implements IPersonService { + private final EmployerService employerService; + private final EmployeeService employeeService; + public EmployerService getEmployerService() { return employerService; } public EmployeeService getEmployeeService() { return employeeService; } - private final EmployerService employerService; - private final EmployeeService employeeService;src/main/java/com/fiscalapi/services/EmployeeService.java (2)
7-9:httpClientshould beprivate final.The field is injected via constructor and never reassigned, so it should be declared
private finalfor immutability and encapsulation.Suggested fix
- IFiscalApiHttpClient httpClient; + private final IFiscalApiHttpClient httpClient;
20-40: Method names should use camelCase per Java conventions.
GetById,Create,Update, andDeleteuse PascalCase which is unconventional for Java methods. Standard practice is camelCase:getById,create,update,delete.Suggested fix
- public ApiResponse<EmployeeData> GetById(String id) { + public ApiResponse<EmployeeData> getById(String id) { String endpoint = buildEndpoint(id); return httpClient.get(endpoint, EmployeeData.class); } - public ApiResponse<EmployeeData>Create(EmployeeData requestModel) { + public ApiResponse<EmployeeData> create(EmployeeData requestModel) { String endpoint = buildEndpoint(requestModel.getEmployeePersonId()); return httpClient.post(endpoint, requestModel, EmployeeData.class); } - public ApiResponse<EmployeeData> Update(EmployeeData requestModel) { + public ApiResponse<EmployeeData> update(EmployeeData requestModel) { String endpoint = buildEndpoint(requestModel.getEmployeePersonId()); return httpClient.put(endpoint, requestModel, EmployeeData.class); } - public ApiResponse<Boolean> Delete(String id) { + public ApiResponse<Boolean> delete(String id) { return httpClient.delete(buildEndpoint(id)); }src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollEarning.java (1)
6-62: Field declarations are split between top and bottom of the class.Fields
concept,taxedAmount,exemptAmount, andovertimeare declared at lines 58-61, after their accessor methods. Java convention is to declare all instance fields together at the top of the class for readability.Suggested reorganization
public class PayrollEarning { private String earningTypeCode; private String code; + private String concept; + private BigDecimal taxedAmount; + private BigDecimal exemptAmount; + private List<PayrollEarningOvertime> overtime; public String getEarningTypeCode() { return earningTypeCode; @@ -54,9 +58,4 @@ public void setOvertime(List<PayrollEarningOvertime> overtime) { this.overtime = overtime; } - - private String concept; - private BigDecimal taxedAmount; - private BigDecimal exemptAmount; - private List<PayrollEarningOvertime> overtime; }src/main/java/com/fiscalapi/models/downloading/DownloadRequest.java (1)
235-236: Adopt standard JavaBeans setter method names via overloading.
setXFromLocalDateTimeis non-standard. JavaBeans tooling (Spring, BeanUtils, IDEs) recognizes only the patternpublic void setX(T)for propertyXof typeT. Overload existingsetX(String)methods withsetX(LocalDateTime)variants instead, which allows both signatures while maintaining bean introspection compatibility.♻️ Overload example (apply to all four methods)
-public void setStartDateFromLocalDateTime(LocalDateTime startDate) { +public void setStartDate(LocalDateTime startDate) { this.startDate = startDate; }Also applies to: 256–257 (
setEndDateFromLocalDateTime), 442–443 (setLastAttemptDateFromLocalDateTime), 460–462 (setNextAttemptDateFromLocalDateTime)
| import com.fasterxml.jackson.databind.annotation.JsonSerialize; | ||
| import com.fasterxml.jackson.datatype.jsr310.deser.LocalDateTimeDeserializer; | ||
| import com.fasterxml.jackson.datatype.jsr310.ser.LocalDateTimeSerializer; | ||
| import com.fiscalapi.OptUtil; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the OptUtil class
find . -name "OptUtil.java" -type fRepository: FiscalAPI/fiscalapi-java
Length of output: 110
🏁 Script executed:
# Find the DownloadRequest.java file
find . -name "DownloadRequest.java" -type fRepository: FiscalAPI/fiscalapi-java
Length of output: 137
🏁 Script executed:
cat -n ./src/main/java/com/fiscalapi/OptUtil.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 1517
🏁 Script executed:
# Get line count to check size before reading
wc -l ./src/main/java/com/fiscalapi/models/downloading/DownloadRequest.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 141
🏁 Script executed:
# Read the relevant sections of DownloadRequest.java
sed -n '1,50p' ./src/main/java/com/fiscalapi/models/downloading/DownloadRequest.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 2041
🏁 Script executed:
# Check the specific lines mentioned in the review
sed -n '226,230p' ./src/main/java/com/fiscalapi/models/downloading/DownloadRequest.java
sed -n '247,251p' ./src/main/java/com/fiscalapi/models/downloading/DownloadRequest.java
sed -n '438,440p' ./src/main/java/com/fiscalapi/models/downloading/DownloadRequest.java
sed -n '456,458p' ./src/main/java/com/fiscalapi/models/downloading/DownloadRequest.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 814
🏁 Script executed:
# Check the field declarations for these date fields
sed -n '1,100p' ./src/main/java/com/fiscalapi/models/downloading/DownloadRequest.java | grep -A 5 -B 2 "LocalDateTime"Repository: FiscalAPI/fiscalapi-java
Length of output: 1947
🏁 Script executed:
# Check for lastAttemptDate and nextAttemptDate field declarations
sed -n '50,150p' ./src/main/java/com/fiscalapi/models/downloading/DownloadRequest.java | grep -A 3 -B 1 "AttemptDate\|CreatedDate"Repository: FiscalAPI/fiscalapi-java
Length of output: 534
🏁 Script executed:
# Let me verify the exact behavior by checking what LocalDateTime.MIN is and how it serializes
python3 << 'EOF'
from datetime import datetime, timezone
import json
# LocalDateTime.MIN in Java is: −999999999-01-01T00:00:00
# This represents year -999999999, which would serialize to something like -999999999-01-01T00:00:00.000
# The SAT (Servicio de Administración Tributaria) is Mexico's tax authority
# Their date format should be realistic dates, not ancient negative years
# Let's confirm what the pattern would produce
pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS"
java_min_year = -999999999
java_min_date = f"{java_min_year:0>10}-01-01T00:00:00.000"
print(f"LocalDateTime.MIN would serialize to: {java_min_date}")
print(f"This is clearly invalid for SAT date ranges (should be recent dates)")
print(f"\nThe review comment is correct: null/empty should map to null, not LocalDateTime.MIN")
EOFRepository: FiscalAPI/fiscalapi-java
Length of output: 289
🏁 Script executed:
# Verify there are no other places where these date setters are being called with proper null handling
rg "setStartDate|setEndDate|setLastAttemptDate|setNextAttemptDate" --type java -A 2 -B 2Repository: FiscalAPI/fiscalapi-java
Length of output: 4925
Guard null/empty date strings before parsing to prevent invalid SAT dates.
OptUtil.formatInputDateToSATFormat returns LocalDateTime.MIN (year -999999999) for null/empty input, which serializes to -999999999-01-01T00:00:00.000—an invalid date for SAT operations. Map null/blank input to null instead.
Apply this guard to all four String-based date setters:
✅ Required fix pattern
public void setStartDate(String startDate) {
- this.startDate = OptUtil.formatInputDateToSATFormat(startDate);
+ if (startDate == null || startDate.isEmpty()) {
+ this.startDate = null;
+ return;
+ }
+ this.startDate = OptUtil.formatInputDateToSATFormat(startDate);
}Also applies to setEndDate (226-230), setLastAttemptDate (247-251), and setNextAttemptDate (438-440).
🤖 Prompt for AI Agents
In `@src/main/java/com/fiscalapi/models/downloading/DownloadRequest.java` at line
7, DownloadRequest's String-based setters (setStartDate, setEndDate,
setLastAttemptDate, setNextAttemptDate) call OptUtil.formatInputDateToSATFormat
and end up storing LocalDateTime.MIN for null/blank input; guard the input by
checking if the incoming String is null or blank and set the corresponding field
to null in that case, otherwise call OptUtil.formatInputDateToSATFormat and
assign the parsed value; update the logic inside the four methods
(setStartDate(String), setEndDate(String), setLastAttemptDate(String),
setNextAttemptDate(String)) to perform this null/blank check before parsing so
SAT-invalid -999999999 dates are not produced.
| @JsonProperty("invoiceDate") | ||
| public void setSatInvoiceDate(String satDate) { | ||
| if (satDate == null || satDate.isEmpty()) { | ||
| this.invoiceDate = null; | ||
| return; | ||
| } | ||
| try { | ||
| this.invoiceDate = LocalDateTime.parse(satDate, SAT_DATE_FORMAT_IN); | ||
| } catch (DateTimeParseException e) { | ||
| try { | ||
| ZonedDateTime zdt = ZonedDateTime.parse(satDate); | ||
| this.invoiceDate = zdt.toLocalDateTime(); | ||
| } catch (DateTimeParseException e2) { | ||
| throw new IllegalArgumentException("Formato de fecha inválido: " + satDate, e); | ||
| } | ||
| } | ||
| this.invoiceDate = com.fiscalapi.OptUtil.formatInputDateToSATFormat(satDate); | ||
| } |
There was a problem hiding this comment.
Avoid serializing LocalDateTime.MIN for null/empty inputs.
OptUtil returns LocalDateTime.MIN when the input is null/empty. That value bypasses the null check in the getters and gets serialized as a bogus date. Treat MIN as “unset” (or short‑circuit null inputs) to preserve previous semantics.
🛠️ Proposed fix
`@JsonProperty`("invoiceDate")
public void setSatInvoiceDate(String satDate) {
- this.invoiceDate = com.fiscalapi.OptUtil.formatInputDateToSATFormat(satDate);
+ LocalDateTime parsed = com.fiscalapi.OptUtil.formatInputDateToSATFormat(satDate);
+ this.invoiceDate = LocalDateTime.MIN.equals(parsed) ? null : parsed;
}
`@JsonProperty`("satCertificationDate")
public void setSatCertificationDateFormatted(String satDate) {
- this.satCertificationDate = com.fiscalapi.OptUtil.formatInputDateToSATFormat(satDate);
+ LocalDateTime parsed = com.fiscalapi.OptUtil.formatInputDateToSATFormat(satDate);
+ this.satCertificationDate = LocalDateTime.MIN.equals(parsed) ? null : parsed;
}
`@JsonProperty`("cancellationDate")
public void setSatCancellationDate(String satDate) {
- this.cancellationDate = com.fiscalapi.OptUtil.formatInputDateToSATFormat(satDate);
+ LocalDateTime parsed = com.fiscalapi.OptUtil.formatInputDateToSATFormat(satDate);
+ this.cancellationDate = LocalDateTime.MIN.equals(parsed) ? null : parsed;
}Also applies to: 178-180, 225-227
🤖 Prompt for AI Agents
In `@src/main/java/com/fiscalapi/models/downloading/MetadataItem.java` around
lines 155 - 158, setSatInvoiceDate currently assigns the result of
OptUtil.formatInputDateToSATFormat(satDate) directly, but OptUtil returns
LocalDateTime.MIN for null/empty inputs which then gets serialized as a bogus
date; change setSatInvoiceDate to short‑circuit null/empty inputs or check the
returned LocalDateTime and set this.invoiceDate to null when the input is
null/empty or when the formatted value equals LocalDateTime.MIN, otherwise
assign the formatted value; apply the same pattern to the other MetadataItem
setters that call OptUtil.formatInputDateToSATFormat so any SAT date formatter
result equal to LocalDateTime.MIN is treated as unset (null).
| import com.fiscalapi.common.CatalogDto; | ||
| import com.fiscalapi.models.invoicing.paymentComplement.InvoicePayment; | ||
|
|
||
| import java.math.BigDecimal; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Java BigDecimal constructor String null empty exception behavior
💡 Result:
new BigDecimal((String) null)→ throws NullPointerException (theStringreference isnull). The BigDecimal API specifies that constructors/methods throwNullPointerExceptionwhen passed anullobject reference. [2]new BigDecimal("")(empty string) → throws NumberFormatException, because the accepted grammar requires at least one digit in either the integer part or the fraction. [1]new BigDecimal(" ")(whitespace-only) → also NumberFormatException (extraneous characters like whitespace are not allowed). [3]
Sources: Oracle BigDecimal Javadoc (Java SE 8, Java SE 22), Oracle Java ME BigDecimal spec. [1][2][3]
🏁 Script executed:
fd Invoice.java -t f | head -1 | xargs cat -n | head -100Repository: FiscalAPI/fiscalapi-java
Length of output: 3674
🏁 Script executed:
fd Invoice.java -t f | head -1 | xargs sed -n '109,150p'Repository: FiscalAPI/fiscalapi-java
Length of output: 1011
🏁 Script executed:
fd Invoice.java -t f | head -1 | xargs sed -n '310,330p'Repository: FiscalAPI/fiscalapi-java
Length of output: 579
Add null/empty guards to BigDecimal String setters to prevent deserialization failures.
new BigDecimal(String) throws NullPointerException on null input and NumberFormatException on empty or whitespace-only strings. The four setters (setSubtotal, setDiscount, setTotal, setExchangeRate) currently lack guards, which breaks deserialization when optional fields are null or blank.
Apply the suggested guard pattern to all four methods:
Guard pattern
public void setSubtotal(String subtotal) {
- this.subtotal = new BigDecimal(subtotal);
+ this.subtotal = (subtotal == null || subtotal.isEmpty())
+ ? null
+ : new BigDecimal(subtotal);
}
public void setDiscount(String discount) {
- this.discount = new BigDecimal(discount);
+ this.discount = (discount == null || discount.isEmpty())
+ ? null
+ : new BigDecimal(discount);
}
public void setTotal(String total) {
- this.total = new BigDecimal(total);
+ this.total = (total == null || total.isEmpty())
+ ? null
+ : new BigDecimal(total);
}
public void setExchangeRate(String exchangeRate) {
- this.exchangeRate = new BigDecimal(exchangeRate);
+ this.exchangeRate = (exchangeRate == null || exchangeRate.isEmpty())
+ ? null
+ : new BigDecimal(exchangeRate);
}🤖 Prompt for AI Agents
In `@src/main/java/com/fiscalapi/models/invoicing/Invoice.java` at line 8, In
class Invoice, the setters setSubtotal, setDiscount, setTotal, and
setExchangeRate should guard against null or blank string input before calling
new BigDecimal(String); update each setter to check if the input string is null
or blank (e.g., null or trim().isEmpty()) and if so set the corresponding
BigDecimal field to null (or leave it unset), otherwise construct new BigDecimal
from the trimmed string and assign it—apply this exact pattern to setSubtotal,
setDiscount, setTotal, and setExchangeRate to avoid
NullPointerException/NumberFormatException during deserialization.
| @JsonProperty("date") | ||
| public void setSatDate(String satDate) { | ||
| if (satDate == null || satDate.isEmpty()) { | ||
| this.date = null; | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // Intenta primero parsearlo como LocalDateTime | ||
| this.date = LocalDateTime.parse(satDate, SAT_DATE_FORMAT_IN); | ||
| } catch (DateTimeParseException e) { | ||
| try { | ||
| // Si falla, intenta parsearlo como ZonedDateTime y convertirlo a LocalDateTime | ||
| ZonedDateTime zdt = ZonedDateTime.parse(satDate); | ||
| this.date = zdt.toLocalDateTime(); | ||
| } catch (DateTimeParseException e2) { | ||
| throw new IllegalArgumentException("Formato de fecha inválido: " + satDate + | ||
| " (debe ser compatible con el formato yyyy-MM-ddTHH:mm:ss)", e); | ||
| } | ||
| } | ||
| this.date = com.fiscalapi.OptUtil.formatInputDateToSATFormat(satDate); | ||
| } | ||
|
|
||
| /** | ||
| * @param dateString Fecha y hora de expedición como String en formato SAT | ||
| */ | ||
| public void setDate(String dateString) { | ||
| this.date = com.fiscalapi.OptUtil.formatInputDateToSATFormat(dateString); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the Invoice.java file around lines 216-226
fd "Invoice.java" --type f | head -5Repository: FiscalAPI/fiscalapi-java
Length of output: 270
🌐 Web query:
Jackson conflicting setter definitions for a single property with @JsonProperty``
💡 Result:
That exception means Jackson has discovered more than one “setter” candidate for the same logical JSON property name, so it can’t decide which one to use (overloaded setX(...), Lombok-generated + manual setters, or two fields/methods both mapped to the same @JsonProperty name). Stack Overflow examples show this both for overloaded setters and for two different members annotated with the same @JsonProperty("value") (which produces conflicting accessors) [1][2].
Fixes (pick one)
- Have exactly one Jackson-mapped setter
- Keep one setter as the JSON setter and make the others not setters (rename them), or ignore them.
- The most direct approach is to mark the intended one with
@JsonSetter(and keep the others un-mapped/renamed). This resolves ambiguity in the common “overloaded setter” case [1].
public class TestDTO {
private CustomWrapper wrappedValues;
`@JsonSetter`("wrappedValues")
public void setWrappedValues(CustomWrapper v) { this.wrappedValues = v; }
// rename helper methods so they are not seen as setters
public TestDTO withWrappedValues(List<WrappedObject> v) { ...; return this; }
}- Don’t map two different fields/getters to the same JSON name
- If you have something like:
Jackson will treat that as two accessors for the same property and fail; use one property (e.g.,
`@JsonProperty`("value") private String value; `@JsonProperty`("value") private Value valueObj;
JsonNode,Object, or a custom deserializer) instead [2].
- If annotations “don’t work”, verify you’re actually using Jackson
- In Jersey, another JSON provider can take precedence unless Jackson is explicitly registered; one answer suggests registering
JacksonFeaturewhen annotations appear ignored [3]. - Also ensure you’re not mixing Jackson packages/providers (a real-world cause reported in the overloaded-setter thread) [1].
Sources: [1] [2] [3]
🏁 Script executed:
# Read the Invoice.java file around lines 216-226
sed -n '200,240p' src/main/java/com/fiscalapi/models/invoicing/Invoice.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 1313
🏁 Script executed:
# Find and read OptUtil to understand formatInputDateToSATFormat
fd "OptUtil.java" --type fRepository: FiscalAPI/fiscalapi-java
Length of output: 108
🏁 Script executed:
# Read OptUtil.java to see formatInputDateToSATFormat
cat -n src/main/java/com/fiscalapi/OptUtil.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 1517
Remove Jackson conflict: both setSatDate and setDate target the same property.
setSatDate() is explicitly mapped to "date" via @JsonProperty, while setDate() implicitly targets the same property by JavaBean naming convention. Jackson will throw a conflict exception during deserialization. Add @JsonIgnore to setDate(String) to mark it as non-JSON and add null/empty guards to prevent returning LocalDateTime.MIN:
Suggested fix
+@JsonIgnore
public void setDate(String dateString) {
+ if (dateString == null || dateString.isEmpty()) {
+ this.date = null;
+ return;
+ }
this.date = com.fiscalapi.OptUtil.formatInputDateToSATFormat(dateString);
}🤖 Prompt for AI Agents
In `@src/main/java/com/fiscalapi/models/invoicing/Invoice.java` around lines 216 -
226, Remove the Jackson setter conflict by annotating the plain JavaBean setter
setDate(String) with `@JsonIgnore` so only setSatDate(String) (annotated with
`@JsonProperty`("date")) is used for JSON deserialization; additionally, inside
setDate(String) and setSatDate(String) add null/empty guards before calling
com.fiscalapi.OptUtil.formatInputDateToSATFormat(...) and only assign to
this.date when the input is non-null/non-empty (to avoid
formatInputDateToSATFormat returning LocalDateTime.MIN or otherwise unintended
values).
| @@ -1,19 +1,22 @@ | |||
| package com.fiscalapi.models.invoicing; | |||
|
|
|||
| import java.math.BigDecimal; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "InvoiceItem.java" -type fRepository: FiscalAPI/fiscalapi-java
Length of output: 131
🏁 Script executed:
cat -n ./src/main/java/com/fiscalapi/models/invoicing/InvoiceItem.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 3388
🌐 Web query:
Java BigDecimal(String) null empty string exception behavior
💡 Result:
new BigDecimal((String) null)throws NullPointerException (BigDecimal constructors/methods throw NPE when any input parameter isnull). [1]new BigDecimal("")(empty string) throws NumberFormatException because theStringform must contain at least one digit and otherwise is not a validBigDecimalrepresentation. [1]- Similarly, strings that are only whitespace (e.g.,
" ") also throw NumberFormatException unless you trim/validate first, because the constructor does not accept non-digit content. [1]
Sources
[1] Java BigDecimal Javadoc (Constructor BigDecimal(String) and class contract), Oracle Java SE 22 docs. (docs.oracle.com)
Citations:
String BigDecimal setters must handle null/blank inputs to prevent deserialization failures.
The new BigDecimal(String) constructor throws NullPointerException for null and NumberFormatException for empty or whitespace-only strings. Apply the guard pattern to setQuantity() (line 36), setDiscount() (line 42), and setUnitPrice() (line 60):
Guard pattern
public void setQuantity(String quantity) {
- this.quantity = new BigDecimal(quantity);
+ this.quantity = (quantity == null || quantity.isEmpty())
+ ? null
+ : new BigDecimal(quantity);
}
public void setDiscount(String discount) {
- this.discount = new BigDecimal(discount);
+ this.discount = (discount == null || discount.isEmpty())
+ ? null
+ : new BigDecimal(discount);
}
public void setUnitPrice(String unitPrice) {
- this.unitPrice = new BigDecimal(unitPrice);
+ this.unitPrice = (unitPrice == null || unitPrice.isEmpty())
+ ? null
+ : new BigDecimal(unitPrice);
}🤖 Prompt for AI Agents
In `@src/main/java/com/fiscalapi/models/invoicing/InvoiceItem.java` at line 3, In
InvoiceItem, the String-based setters setQuantity, setDiscount, and setUnitPrice
must guard against null/blank strings before calling new BigDecimal(...) to
avoid NPE/NumberFormatException during deserialization; update each setter to
check if the input is null or input.trim().isEmpty() and if so assign the
corresponding BigDecimal field to null (or a safe default), otherwise call new
BigDecimal(input.trim()) (optionally wrap the construction in a try/catch to
rethrow a clear IllegalArgumentException on invalid number formats).
| private String fromPersonId; | ||
| private String toPersonId; | ||
| private int amount; | ||
| public String comments; |
There was a problem hiding this comment.
Field comments is declared public instead of private.
This breaks encapsulation and is inconsistent with the other fields in the class which are all private with corresponding getters/setters.
🐛 Proposed fix
- public String comments;
+ private String comments;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public String comments; | |
| private String comments; |
🤖 Prompt for AI Agents
In `@src/main/java/com/fiscalapi/models/StampTransactionParams.java` at line 8,
The field "comments" in class StampTransactionParams is declared public,
breaking encapsulation and differing from other private fields; change the
declaration of the comments field to private and add standard accessor methods
getComments() and setComments(String comments) in StampTransactionParams so it
matches the existing getter/setter pattern used by other fields and preserves
API/serialization expectations.
| public ApiResponse<EmployeeData>Create(EmployeeData requestModel) { | ||
| String endpoint = buildEndpoint(requestModel.getEmployeePersonId()); | ||
|
|
||
| return httpClient.post(endpoint, requestModel, EmployeeData.class); | ||
| } | ||
|
|
||
| public ApiResponse<EmployeeData> Update(EmployeeData requestModel) { | ||
| String endpoint = buildEndpoint(requestModel.getEmployeePersonId()); | ||
|
|
||
| return httpClient.put(endpoint, requestModel, EmployeeData.class); | ||
| } |
There was a problem hiding this comment.
Potential NPE if getEmployeePersonId() returns null.
In Create and Update, requestModel.getEmployeePersonId() is passed directly to buildEndpoint(). If the value is null, String.format will produce an endpoint like api/v4/people/null/employee, leading to incorrect API calls rather than a clear failure.
Consider validating the input or throwing an IllegalArgumentException early.
Suggested defensive check
- public ApiResponse<EmployeeData>Create(EmployeeData requestModel) {
- String endpoint = buildEndpoint(requestModel.getEmployeePersonId());
+ public ApiResponse<EmployeeData> create(EmployeeData requestModel) {
+ if (requestModel.getEmployeePersonId() == null) {
+ throw new IllegalArgumentException("employeePersonId must not be null");
+ }
+ String endpoint = buildEndpoint(requestModel.getEmployeePersonId());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ApiResponse<EmployeeData>Create(EmployeeData requestModel) { | |
| String endpoint = buildEndpoint(requestModel.getEmployeePersonId()); | |
| return httpClient.post(endpoint, requestModel, EmployeeData.class); | |
| } | |
| public ApiResponse<EmployeeData> Update(EmployeeData requestModel) { | |
| String endpoint = buildEndpoint(requestModel.getEmployeePersonId()); | |
| return httpClient.put(endpoint, requestModel, EmployeeData.class); | |
| } | |
| public ApiResponse<EmployeeData> create(EmployeeData requestModel) { | |
| if (requestModel.getEmployeePersonId() == null) { | |
| throw new IllegalArgumentException("employeePersonId must not be null"); | |
| } | |
| String endpoint = buildEndpoint(requestModel.getEmployeePersonId()); | |
| return httpClient.post(endpoint, requestModel, EmployeeData.class); | |
| } | |
| public ApiResponse<EmployeeData> Update(EmployeeData requestModel) { | |
| String endpoint = buildEndpoint(requestModel.getEmployeePersonId()); | |
| return httpClient.put(endpoint, requestModel, EmployeeData.class); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/com/fiscalapi/services/EmployeeService.java` around lines 26 -
36, Add null-checks for requestModel and for requestModel.getEmployeePersonId()
at the start of both Create and Update methods and throw an
IllegalArgumentException with a clear message if either is null; specifically
validate inside Create(...) and Update(...) before calling
buildEndpoint(requestModel.getEmployeePersonId()), e.g., ensure requestModel !=
null and requestModel.getEmployeePersonId() != null and throw
IllegalArgumentException("...") when validation fails so buildEndpoint and
downstream httpClient calls never receive "null" as the id.
| @Override | ||
| public ApiResponse<Invoice> create(Invoice model) { | ||
|
|
||
| String path =""; | ||
|
|
||
| switch (model.getTypeCode()) { | ||
| case "I": | ||
| path = "income"; | ||
| break; | ||
| case "E": | ||
| path = "credit-note"; | ||
| break; | ||
| case "P": | ||
| path = "payment"; | ||
| break; | ||
| default: | ||
| throw new IllegalArgumentException("Invalid invoice type"); | ||
| } | ||
|
|
||
| String endpoint = buildEndpoint(path, null); | ||
| return httpClient.post(endpoint, model, Invoice.class); | ||
|
|
||
| return httpClient.post(buildEndpoint("", null), model, Invoice.class); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the InvoiceService file
find . -name "InvoiceService.java" -type fRepository: FiscalAPI/fiscalapi-java
Length of output: 126
🏁 Script executed:
# Search for test files related to InvoiceService
find . -name "*InvoiceService*Test*.java" -o -name "*Test*InvoiceService*.java"Repository: FiscalAPI/fiscalapi-java
Length of output: 50
🏁 Script executed:
# Look for references to invoice types and endpoint patterns
rg -n "income|credit-note|payment" --type java | head -30Repository: FiscalAPI/fiscalapi-java
Length of output: 3415
🏁 Script executed:
# Check for buildEndpoint method definition to understand how it constructs URLs
rg -n "buildEndpoint" --type java -A 5 | head -40Repository: FiscalAPI/fiscalapi-java
Length of output: 3295
🏁 Script executed:
# Search for any comments or documentation about invoice types
rg -n "invoice.*type|type.*invoice" --type java -i | head -20Repository: FiscalAPI/fiscalapi-java
Length of output: 2366
🏁 Script executed:
# Read the full InvoiceService.java file
cat -n src/main/java/com/fiscalapi/services/InvoiceService.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 2558
🏁 Script executed:
# Check git history for the InvoiceService.java to see what changed
git log --oneline -20 -- src/main/java/com/fiscalapi/services/InvoiceService.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 115
🏁 Script executed:
# Look at the parent class/interface to understand the contract
rg -n "class InvoiceService|interface.*Service" --type java | grep -A2 InvoiceServiceRepository: FiscalAPI/fiscalapi-java
Length of output: 871
🏁 Script executed:
# Check the IInvoiceService interface to understand the contract
cat -n src/main/java/com/fiscalapi/abstractions/IInvoiceService.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 1113
🏁 Script executed:
# Check BaseFiscalApiService to understand buildEndpoint behavior
cat src/main/java/com/fiscalapi/abstractions/BaseFiscalApiService.java | head -100Repository: FiscalAPI/fiscalapi-java
Length of output: 3343
🏁 Script executed:
# Search for any invoice type codes or routing logic anywhere in the codebase
rg "\"I\"|\"E\"|\"P\"" --type java -B2 -A2 | grep -i "invoice\|type" | head -20Repository: FiscalAPI/fiscalapi-java
Length of output: 1743
🏁 Script executed:
# Check if there's a git diff or any uncommitted changes
git statusRepository: FiscalAPI/fiscalapi-java
Length of output: 291
🏁 Script executed:
# Check if there's git history with more detail
git log --all --oneline src/main/java/com/fiscalapi/services/InvoiceService.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 309
🏁 Script executed:
# Search for any switch statements or type-based conditional logic in InvoiceService or related files
rg "switch|case.*[IEP]" --type java src/main/java/com/fiscalapi/services/ -A3 -B3Repository: FiscalAPI/fiscalapi-java
Length of output: 50
🏁 Script executed:
# Look for any test files that might show the expected behavior
find . -type f -name "*.java" | xargs grep -l "InvoiceService" | grep -i testRepository: FiscalAPI/fiscalapi-java
Length of output: 50
🏁 Script executed:
# Check the specific commits that updated invoice service
git show 5e7c019:src/main/java/com/fiscalapi/services/InvoiceService.java 2>/dev/null | head -80Repository: FiscalAPI/fiscalapi-java
Length of output: 2563
🏁 Script executed:
# Check an older version to see if there was type-based routing
git show 1160bca:src/main/java/com/fiscalapi/services/InvoiceService.java 2>/dev/null | head -80Repository: FiscalAPI/fiscalapi-java
Length of output: 1656
🏁 Script executed:
# Look at the Invoice model to understand if typeCode is a property
rg "typeCode|getTypeCode|setTypeCode" --type java -B2 -A2Repository: FiscalAPI/fiscalapi-java
Length of output: 25615
Restore type-based endpoint routing for invoice creation.
The create method was simplified to always POST to the base /invoices endpoint, removing the switch statement that previously routed requests to type-specific endpoints (income for type "I", credit-note for type "E", payment for type "P"). Git history confirms this routing logic existed in earlier commits.
This change breaks invoice creation for credit notes (E) and payments (P) unless the backend API was recently updated to accept all types at a unified endpoint. Restore the type-based routing:
Expected implementation
`@Override`
public ApiResponse<Invoice> create(Invoice model) {
String path = "";
switch (model.getTypeCode()) {
case "I":
path = "income";
break;
case "E":
path = "credit-note";
break;
case "P":
path = "payment";
break;
default:
throw new IllegalArgumentException("Invalid invoice type");
}
String endpoint = buildEndpoint(path, null);
return httpClient.post(endpoint, model, Invoice.class);
}🤖 Prompt for AI Agents
In `@src/main/java/com/fiscalapi/services/InvoiceService.java` around lines 60 -
63, The create method in InvoiceService currently always posts to the base
invoices endpoint; restore type-based routing by inspecting model.getTypeCode()
inside create and map "I" -> "income", "E" -> "credit-note", "P" -> "payment",
then call buildEndpoint(mappedPath, null) and pass that endpoint to
httpClient.post(model, Invoice.class); for any other type throw an
IllegalArgumentException("Invalid invoice type") so credit notes and payments
route to their correct endpoints.
| @Override | ||
| public ApiResponse<Boolean> TransferStamps(StampTransactionParams requestModel) { | ||
| return httpClient.post(buildEndpoint("", null), requestModel, Boolean.class); | ||
| } | ||
|
|
||
| @Override | ||
| public ApiResponse<Boolean> WithdrawStamps(StampTransactionParams requestModel) { | ||
| return httpClient.post(buildEndpoint("", null), requestModel, Boolean.class); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find StampTransactionParams definition
find . -type f -name "*.java" | xargs grep -l "class StampTransactionParams"Repository: FiscalAPI/fiscalapi-java
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Find the StampService file and examine its context
find . -path "*/src/main/java/com/fiscalapi/services/StampService.java" -type f | head -1Repository: FiscalAPI/fiscalapi-java
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Search for buildEndpoint method definition
find . -type f -name "*.java" | xargs grep -l "buildEndpoint" | head -5Repository: FiscalAPI/fiscalapi-java
Length of output: 393
🏁 Script executed:
cat -n ./src/main/java/com/fiscalapi/models/StampTransactionParams.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 1215
🏁 Script executed:
cat -n ./src/main/java/com/fiscalapi/services/StampService.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 1465
🏁 Script executed:
grep -A 10 "buildEndpoint" ./src/main/java/com/fiscalapi/abstractions/BaseFiscalApiService.java | head -20Repository: FiscalAPI/fiscalapi-java
Length of output: 744
🏁 Script executed:
find . -type f -name "IStampService.java" | xargs cat -nRepository: FiscalAPI/fiscalapi-java
Length of output: 557
🏁 Script executed:
grep -r "buildEndpoint" ./src/main/java/com/fiscalapi/services/*.java | grep -v "^Binary" | head -20Repository: FiscalAPI/fiscalapi-java
Length of output: 2438
🏁 Script executed:
find . -type f -name "*.java" -path "*/services/*" | head -3 | xargs grep -A 5 "public ApiResponse.*post.*buildEndpoint"Repository: FiscalAPI/fiscalapi-java
Length of output: 52
🏁 Script executed:
find . -type f \( -name "README*" -o -name "*.md" -o -name "*.txt" \) | head -10 | xargs grep -l -i "stamp\|transfer\|withdraw" 2>/dev/nullRepository: FiscalAPI/fiscalapi-java
Length of output: 81
🏁 Script executed:
find . -type f -name "*.json" -o -name "*.yaml" -o -name "*.yml" | head -10Repository: FiscalAPI/fiscalapi-java
Length of output: 98
🏁 Script executed:
# Check InvoiceService for comparison of different endpoint patterns
sed -n '20,60p' ./src/main/java/com/fiscalapi/services/InvoiceService.javaRepository: FiscalAPI/fiscalapi-java
Length of output: 1269
Both methods must use distinct endpoints or include an action field.
Lines 21-29: TransferStamps and WithdrawStamps both POST to the same endpoint (buildEndpoint("", null)) with identical request structure. StampTransactionParams contains no action/type field to differentiate the operations. Based on the pattern in other services (e.g., InvoiceService uses buildEndpoint("status"), buildEndpoint("pdf"), buildEndpoint("send") for distinct operations), consider either using separate endpoint paths (e.g., "transfer" and "withdraw") or adding a transaction type field to the request model so the API can distinguish between these operations.
🤖 Prompt for AI Agents
In `@src/main/java/com/fiscalapi/services/StampService.java` around lines 21 - 29,
TransferStamps and WithdrawStamps both post to buildEndpoint("", null) with
identical StampTransactionParams so the backend cannot distinguish actions;
update these methods to call distinct endpoints (e.g., use
buildEndpoint("transfer", null) in TransferStamps and buildEndpoint("withdraw",
null) in WithdrawStamps) or alternately add a transactionType/action field to
StampTransactionParams and set it appropriately before posting, ensuring the
chosen approach is implemented consistently in the TransferStamps and
WithdrawStamps methods and any consumers of StampTransactionParams.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/com/fiscalapi/OptUtil.java`:
- Around line 20-26: The catch block that parses into ZonedDateTime currently
throws a new IllegalArgumentException using the wrong cause variable; replace
the cause from 'e' to the actual ZonedDateTime parse exception 'e2' so the
thrown IllegalArgumentException includes the correct underlying
DateTimeParseException (update the throw in the catch for DateTimeParseException
to use e2), keeping the existing message that references stringDate and the
expected format and leaving parsedDate/ZonedDateTime logic unchanged.
🧹 Nitpick comments (1)
src/main/java/com/fiscalapi/OptUtil.java (1)
9-9: Make the utility class non‑instantiable.
This class is a pure static utility; add a private constructor and mark it final to prevent accidental instantiation.♻️ Proposed refactor
-public class OptUtil { +public final class OptUtil { + private OptUtil() { + // Utility class + } public static LocalDateTime formatInputDateToSATFormat(String stringDate) {
| try { | ||
| // Si falla, intenta parsearlo como ZonedDateTime y convertirlo a LocalDateTime | ||
| ZonedDateTime zdt = ZonedDateTime.parse(stringDate); | ||
| parsedDate = zdt.toLocalDateTime(); | ||
| } catch (DateTimeParseException e2) { | ||
| throw new IllegalArgumentException("Formato de fecha inválido: " + stringDate + | ||
| " (debe ser compatible con el formato yyyy-MM-ddTHH:mm:ss)", e); |
There was a problem hiding this comment.
Use the actual failing parse exception as the cause.
When the ZonedDateTime parse fails, the thrown IllegalArgumentException currently attaches the first parse exception, which can mislead debugging.
🧩 Proposed fix
} catch (DateTimeParseException e2) {
throw new IllegalArgumentException("Formato de fecha inválido: " + stringDate +
- " (debe ser compatible con el formato yyyy-MM-ddTHH:mm:ss)", e);
+ " (debe ser compatible con el formato yyyy-MM-ddTHH:mm:ss)", e2);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| // Si falla, intenta parsearlo como ZonedDateTime y convertirlo a LocalDateTime | |
| ZonedDateTime zdt = ZonedDateTime.parse(stringDate); | |
| parsedDate = zdt.toLocalDateTime(); | |
| } catch (DateTimeParseException e2) { | |
| throw new IllegalArgumentException("Formato de fecha inválido: " + stringDate + | |
| " (debe ser compatible con el formato yyyy-MM-ddTHH:mm:ss)", e); | |
| try { | |
| // Si falla, intenta parsearlo como ZonedDateTime y convertirlo a LocalDateTime | |
| ZonedDateTime zdt = ZonedDateTime.parse(stringDate); | |
| parsedDate = zdt.toLocalDateTime(); | |
| } catch (DateTimeParseException e2) { | |
| throw new IllegalArgumentException("Formato de fecha inválido: " + stringDate + | |
| " (debe ser compatible con el formato yyyy-MM-ddTHH:mm:ss)", e2); |
🤖 Prompt for AI Agents
In `@src/main/java/com/fiscalapi/OptUtil.java` around lines 20 - 26, The catch
block that parses into ZonedDateTime currently throws a new
IllegalArgumentException using the wrong cause variable; replace the cause from
'e' to the actual ZonedDateTime parse exception 'e2' so the thrown
IllegalArgumentException includes the correct underlying DateTimeParseException
(update the throw in the catch for DateTimeParseException to use e2), keeping
the existing message that references stringDate and the expected format and
leaving parsedDate/ZonedDateTime logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/main/java/com/fiscalapi/services/EmployerService.java`:
- Around line 24-26: Validate the id in EmployerService.GetById before calling
httpClient: check for null or blank (trimmed) and fail fast (e.g., throw
IllegalArgumentException or return an ApiResponse representing a bad request)
instead of building the endpoint; apply the same null/blank validation to the
other similar endpoint methods in this class (the method around lines 36-37)
that call buildEndpoint and httpClient.get to avoid constructing invalid URLs
and to surface caller errors immediately.
- Around line 28-34: Guard Create and Update against null requestModel and
missing personId: in EmployerService methods Create(EmployerData) and
Update(EmployerData) check that requestModel is not null and that
requestModel.getPersonId() is non-null/ non-empty before calling buildEndpoint
or httpClient.post/put; if validation fails, return a failed ApiResponse (or
throw a documented IllegalArgumentException) with a clear error message rather
than proceeding to buildEndpoint/getPersonId to avoid NPEs and invalid endpoint
calls.
- Around line 20-22: The buildEndpoint method in EmployerService is using the
wrong path; update EmployerService.buildEndpoint(String personId) to construct
the URL with "/employer" instead of "/employee" (i.e., use baseEndpoint and
personId to format "%s/%s/employer") so the service targets the correct Employer
resource; ensure any tests or usages of EmployerService that rely on
buildEndpoint still work with the corrected path.
🧹 Nitpick comments (1)
src/main/java/com/fiscalapi/services/EmployerService.java (1)
12-18: MakehttpClientimmutable.
It’s only set in the constructor; marking itfinalimproves safety and clarity.♻️ Proposed change
- private IFiscalApiHttpClient httpClient; + private final IFiscalApiHttpClient httpClient;
| public ApiResponse<EmployerData> GetById(String id) { | ||
| return httpClient.get(buildEndpoint(id), EmployerData.class); | ||
| } |
There was a problem hiding this comment.
Validate id before issuing requests.
null/blank ids will build invalid URLs and mask caller errors.
🛡️ Proposed change
public ApiResponse<EmployerData> GetById(String id) {
+ if (id == null || id.trim().isEmpty()) {
+ throw new IllegalArgumentException("id is required");
+ }
return httpClient.get(buildEndpoint(id), EmployerData.class);
}
@@
public ApiResponse<Boolean> Delete(String id) {
+ if (id == null || id.trim().isEmpty()) {
+ throw new IllegalArgumentException("id is required");
+ }
return httpClient.delete(buildEndpoint(id));
}Also applies to: 36-37
🤖 Prompt for AI Agents
In `@src/main/java/com/fiscalapi/services/EmployerService.java` around lines 24 -
26, Validate the id in EmployerService.GetById before calling httpClient: check
for null or blank (trimmed) and fail fast (e.g., throw IllegalArgumentException
or return an ApiResponse representing a bad request) instead of building the
endpoint; apply the same null/blank validation to the other similar endpoint
methods in this class (the method around lines 36-37) that call buildEndpoint
and httpClient.get to avoid constructing invalid URLs and to surface caller
errors immediately.
| public ApiResponse<EmployerData>Create(EmployerData requestModel) { | ||
| return httpClient.post(buildEndpoint(requestModel.getPersonId()), requestModel, EmployerData.class); | ||
| } | ||
|
|
||
| public ApiResponse<EmployerData> Update(EmployerData requestModel) { | ||
| return httpClient.put(buildEndpoint(requestModel.getPersonId()), requestModel, EmployerData.class); | ||
| } |
There was a problem hiding this comment.
Guard against null requestModel and missing personId.
Without validation, Create/Update can throw NPEs or call invalid endpoints.
🛡️ Proposed change
public ApiResponse<EmployerData>Create(EmployerData requestModel) {
- return httpClient.post(buildEndpoint(requestModel.getPersonId()), requestModel, EmployerData.class);
+ if (requestModel == null) {
+ throw new IllegalArgumentException("requestModel is required");
+ }
+ String personId = requestModel.getPersonId();
+ if (personId == null || personId.trim().isEmpty()) {
+ throw new IllegalArgumentException("personId is required");
+ }
+ return httpClient.post(buildEndpoint(personId), requestModel, EmployerData.class);
}
public ApiResponse<EmployerData> Update(EmployerData requestModel) {
- return httpClient.put(buildEndpoint(requestModel.getPersonId()), requestModel, EmployerData.class);
+ if (requestModel == null) {
+ throw new IllegalArgumentException("requestModel is required");
+ }
+ String personId = requestModel.getPersonId();
+ if (personId == null || personId.trim().isEmpty()) {
+ throw new IllegalArgumentException("personId is required");
+ }
+ return httpClient.put(buildEndpoint(personId), requestModel, EmployerData.class);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ApiResponse<EmployerData>Create(EmployerData requestModel) { | |
| return httpClient.post(buildEndpoint(requestModel.getPersonId()), requestModel, EmployerData.class); | |
| } | |
| public ApiResponse<EmployerData> Update(EmployerData requestModel) { | |
| return httpClient.put(buildEndpoint(requestModel.getPersonId()), requestModel, EmployerData.class); | |
| } | |
| public ApiResponse<EmployerData>Create(EmployerData requestModel) { | |
| if (requestModel == null) { | |
| throw new IllegalArgumentException("requestModel is required"); | |
| } | |
| String personId = requestModel.getPersonId(); | |
| if (personId == null || personId.trim().isEmpty()) { | |
| throw new IllegalArgumentException("personId is required"); | |
| } | |
| return httpClient.post(buildEndpoint(personId), requestModel, EmployerData.class); | |
| } | |
| public ApiResponse<EmployerData> Update(EmployerData requestModel) { | |
| if (requestModel == null) { | |
| throw new IllegalArgumentException("requestModel is required"); | |
| } | |
| String personId = requestModel.getPersonId(); | |
| if (personId == null || personId.trim().isEmpty()) { | |
| throw new IllegalArgumentException("personId is required"); | |
| } | |
| return httpClient.put(buildEndpoint(personId), requestModel, EmployerData.class); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/com/fiscalapi/services/EmployerService.java` around lines 28 -
34, Guard Create and Update against null requestModel and missing personId: in
EmployerService methods Create(EmployerData) and Update(EmployerData) check that
requestModel is not null and that requestModel.getPersonId() is non-null/
non-empty before calling buildEndpoint or httpClient.post/put; if validation
fails, return a failed ApiResponse (or throw a documented
IllegalArgumentException) with a clear error message rather than proceeding to
buildEndpoint/getPersonId to avoid NPEs and invalid endpoint calls.
1 participant
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores