-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-363 request param to select finalized models by label #364
gh-363 request param to select finalized models by label #364
Conversation
@GBishop-PHSA, thanks, this looks like a good solution, however there's some errors in the tests relating to the path endpoints for folders. I think this may need some more handling for Versioned Folders (which should work like DataModels). https://jenkins.cs.ox.ac.uk/job/Mauro%20Data%20Mapper/job/mdm-core/job/PR-364/1/display/redirect |
@@ -127,7 +127,7 @@ class PathService { | |||
throw new ApiBadRequestException('PS04', "[${rootClass.simpleName}] is not a pathable resource") | |||
} | |||
|
|||
MdmDomain rootResource = securableResourceService.findByParentIdAndPathIdentifier(null, rootNode.getFullIdentifier()) | |||
MdmDomain rootResource = securableResourceService.findByParentIdAndPathIdentifier(null, rootNode.getFullIdentifier(), finalisedOnly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors in tests are due to this method being potentially called on any pathable resource so the overriden method itself would need modifying.
As the PathService is generic, unfortunately I don't think this will work because not all Domains (or SecurableResources) will have a concept of finalised so it wouldn't make sense to add that argument throughout.
If we want to parametrise the paths one option would be to have this parameter as a generic pathParams
Map, then in the appropriate places look for a key of finalised/finalisedOnly. Then we wouldn't have to add the specific finalisation param in Folders which don't support it.
Another option would be to extend the path language to support this with a special character for 'latest finalised version' and then implement that where relevant, although I think that may be more complicated.
@@ -58,7 +58,8 @@ class PathController extends RestfulController<CatalogueItem> implements MdmCont | |||
// Permissions have been checked as part of the interceptor | |||
pathedResource = pathService.findResourceByPathFromRootResource(resource as MdmDomain, params.path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this method pathService.findResourceByPathFromRootResource
also need changing as it looks like a similar route into looking up a domain by its path?
fc5624c
to
48e7f4d
Compare
@joe-crawford I'm having an issue getting the tests to fail locally so I'm not sure how to investigate the failures ./gradlew -Dgradle.functionalTest=true :mdm-core:integrationTest ./gradlew -Dgradle.functionalTest=true :mdm-plugin-datamodel:integrationTest |
Thanks @GBishop-PHSA, this looks good and I'll merge it. I've made some minor tweaks in a branch
The test failures originally were all HibernateOptimisticLockingFailureException or StaleStateException, basically these are sometimes expected failures of the Hibernate optimistic locking strategy that in actual use would need to be retried. They're related to a performance/reliability trade off of Hibernate, if they occurred more often in actual use it would be a problem but usually these happen just on the test server. They've passed after rerunning Jenkins. |
5f8085a
to
74131e7
Compare
@joe-crawford, Thanks for looking at this. I've pulled in your changes and squashed the commits. |
@GBishop-PHSA, Great, thank you! |
gh-363 Provide a path in the API to view the latest finalised datamodel
eg. http://localhost:8080/api/dataModels/path/dm:DM_LABEL?finalised=true