Changes for 1674 #1809
Changes for 1674 #1809
Conversation
catalog/src/main/java/org/killbill/billing/catalog/api/user/DefaultCatalogUserApi.java
Outdated
Show resolved
Hide resolved
| @@ -224,5 +220,23 @@ private InternalTenantContext createInternalTenantContext(final TenantContext te | |||
| return internalCallContextFactory.createInternalTenantContextWithoutAccountRecordId(tenantContext); | |||
| } | |||
|
|
|||
| private ValidationErrors validateCatalogInternal(final String catalogXML, final InternalTenantContext internalTenantContext) throws ValidationException, JAXBException, CatalogApiException, TransformerException, SAXException, IOException { | |||
There was a problem hiding this comment.
Instead of throwing here, could you extract error messages from the exception and add them into the errors?
I'm thinking specifically of errors like:
Entity: line 2: parser error : Opening and ending tag mismatch: effectiveDate line 2 and effectiveate
<effectiveDate>2013-02-08T00:00:00+00:00</effectiveate>
^
Entity: line 4: parser error : Opening and ending tag mismatch: recurringBillingMode line 4 and recurringBillingMde
<recurringBillingMode>IN_ADVANCE</recurringBillingMde>
^
Entity: line 10: parser error : Opening and ending tag mismatch: category line 10 and catgory
<category>BASE</catgory>
or mis-matches in the references (e.g. pricelist with a plan that isn't defined):
Caused by: org.xml.sax.SAXParseException; lineNumber: 52; columnNumber: 11; cvc-id.1: There is no ID/IDREF binding for IDREF 'Standard'.
There was a problem hiding this comment.
Fixed as follows:
- added
e.getLinkedException().getMessage()forJAXBException(Since the error message is present in thelinkedException) - Added
throw new IllegalStateException(e)forTransformerException/IOException/SAXExceptionto keep this consistent with the earlieruploadCatalogcode
Let me know how this looks.
catalog/src/main/java/org/killbill/billing/catalog/DefaultCatalogValidationError.java
Outdated
Show resolved
Hide resolved
@reshmabidikar That should not be the case I think as long as we merge PRs from different repos ( So, I would expect: As long as you ensure that your |
My |
I see, you are correct. I did not realize there is no snapshot generation until we merge your If this is the case, what we could do in the future is create such @reshmabidikar I just merged your api PR so you should be able to use @pierre Are we good from your point of view? |
| } | ||
| catalogCache.clearCatalog(internalTenantContext); |
There was a problem hiding this comment.
This is required. The validateCatalogInternal method adds the new catalog to be validated to the existing VersionedCatalog in order to perform the validation. This results in the VersionedCatalog in the cache being modified during the validation process without an actual catalog upload being performed. So, the cache needs to be cleared after the validation is complete.
There was a problem hiding this comment.
oh, I missed this. This is risky...
Maybe we should clone the catalog first to be safe (using readExternal/writeExternal). @sbrossie thoughts?
profiles/killbill/src/test/java/org/killbill/billing/jaxrs/TestCatalog.java
Show resolved
Hide resolved
| } catch (final ValidationException e) { | ||
| errors.addAll(e.getErrors()); | ||
| } catch (final JAXBException e) { | ||
| errors.add(new ValidationError(e.getLinkedException().getMessage(), DefaultVersionedCatalog.class, "")); |
There was a problem hiding this comment.
Maybe NPE check on e.getLinkedException() just in case?
There was a problem hiding this comment.
Added a null check for e.getLinkedException() LMK if this is what you were referring to.
There was a problem hiding this comment.
I don't see it, maybe the PR was merged too early?
There was a problem hiding this comment.
Yes, the old PR was merged so I've created a new PR: #1813
Includes changes for 1674
Following are the related PRs:
Note that until the above PRs are merged, tests will not be green (After merging, the correct version needs to be specified in the
killbill/pom.xml).