RESTWS-739:Reposting metadata should not persist changes #414
Conversation
|
@gitcliff , can you explain what you were trying to do ?? , From looking at the tests, it seems now metada cannot be updatable after your changes ,because all tests about updating metadata return exceptions. |
| @@ -162,7 +162,7 @@ public void shouldCreateANewOrderType() throws Exception { | |||
| assertEquals(orderType.get("description"), Util.getByPath(newOrder, "description")); | |||
| } | |||
|
|
|||
| @Test | |||
| @Test(expected = RuntimeException.class) | |||
There was a problem hiding this comment.
@gitcliff after your changes , now these tests return Exceptions ?? Thats wrong
There was a problem hiding this comment.
@mozzy11 these tests were run against the update method in DelegatingCrudResource which was allowing reposting of changes metadata but the ticket description says this should not happen any more with metadata hence notifying the client with an exception ,,,thats why the tests that were permitiing the reposting of metadata data initially are now throwing exceptional errors
There was a problem hiding this comment.
@gitcliff ,the tickect talks about forbidding re-posting metadata, not updating metadata.
So idealy updating meta-data shouldnt have any problem
There was a problem hiding this comment.
Yes, @mozzy11 and @gitcliff I haven't looked deeply at the code but looking at the ticket, the idea is, for example, if you post to the Encounter endpoint, although you can change the encounter type of that encounter, you shouldn't be able to inadvertantly change any of the underlying properties (name, etc) of that encounter type. However, you should be able to post directly to the EncounterType endpoint and change the name, etc.
The idea is that you shouldn't be able to by mistake change a piece of metadata when updating some data that refers to that metadata. Hope that makes sense!
There was a problem hiding this comment.
@mozzy11 u can also have a look a this , it concurs with what @mogoodrich was explaining above
| @Test(expected = RuntimeException.class) | ||
| public void shouldThrowAnExceptionWhenEditingAnAnOrderType() throws Exception { | ||
| final String newName = "updated name"; | ||
| SimpleObject orderType = new SimpleObject(); |
There was a problem hiding this comment.
| SimpleObject orderType = new SimpleObject(); | |
| final SimpleObject orderType = new SimpleObject(); |
| final String newName = "updated name"; | ||
| SimpleObject orderType = new SimpleObject(); | ||
| orderType.add("name", newName); | ||
| String json = new ObjectMapper().writeValueAsString(orderType); |
There was a problem hiding this comment.
new ObjectMapper() is resource consuming. Therefore, create one instance for all the relevant unit tests and re-use that instance. 💭
| public void shouldThrowAnExceptionWhenEditingAnAnOrderSet() throws Exception { | ||
|
|
||
| final String editedName = "OrderSet Edited"; | ||
| String json = "{ \"name\":\"" + editedName + "\" }"; |
There was a problem hiding this comment.
I would rename the json to something meaning like editNameJson
| @Test(expected = RuntimeException.class) | ||
| public void shouldThrowAnExceptionWhenEditingAConceptSource() throws Exception { | ||
| final String newName = "updated name"; | ||
| SimpleObject conceptSource = new SimpleObject(); |
There was a problem hiding this comment.
| SimpleObject conceptSource = new SimpleObject(); | |
| final SimpleObject conceptSource = new SimpleObject(); |
| public void shouldThrowAnExceptionWhenEditingAConceptSource() throws Exception { | ||
| final String newName = "updated name"; | ||
| SimpleObject conceptSource = new SimpleObject(); | ||
| conceptSource.add("name", newName); |
There was a problem hiding this comment.
Can't you use a builder for SimpleObject?
| "does not support editing of this type because this is metadata"); | ||
| } | ||
| if (hasTypesDefined()) { | ||
| // if they specify a type discriminator it must match the expected one--type can't be modified |
There was a problem hiding this comment.
Please rewrite this comment in a single sentence. -- is confusing breaker for sentences
| ValidateUtil.validate(delegate); | ||
| delegate = save(delegate); | ||
|
|
||
| SimpleObject ret = (SimpleObject) ConversionUtil.convertToRepresentation(delegate, context.getRepresentation()); |
There was a problem hiding this comment.
what does ret means? maybe give a more self-descriptive name?
| } | ||
|
|
||
| DelegatingResourceHandler<? extends T> handler = getResourceHandler(delegate); | ||
|
|
| } | ||
| } | ||
| } | ||
|
|
| delegate = save(delegate); | ||
|
|
||
| SimpleObject ret = (SimpleObject) ConversionUtil.convertToRepresentation(delegate, context.getRepresentation()); | ||
|
|
ibacher
force-pushed
the
master
branch
2 times, most recently
from
f9c73bb
to
2a4407f
Compare
link:https://issues.openmrs.org/browse/RESTWS-739
Reposting metadata should not persist changes