Skip to content

Conversation

@Ash84
Copy link

@Ash84 Ash84 commented Mar 2, 2023

Hello,
I have been working for a little more than 6 months on the client, to add functions that we needed, me and the Agroclim team at INRAE, for our applications.
I was going to submit the modifications to you last November when I realized that you had also advanced on your side on the project.
My modifications concern several points:

  • Possibility to provide other metadata in addition to those you had already provided
  • Generalization of the DatasetMetadataBlock object and all those that use it or come from it, to be able to easily add fields
  • New metadata fields
  • Easier error returns in the application that uses the client, with the new Exception classes
  • The UsersOperations class that contains a method to retrieve the expiration date of the token used

And other minor improvements, the correct replacement of the license in the classes, renamed some classes to be more accurate and some documentation fixes.

I proceeded to merge on my side to save you this effort, and I propose you this version, where everything is functional.
We used the demo version of https://entrepot.recherche.data.gouv.fr/ , https://demo.recherche.data.gouv.fr/ , which was recently chosen by the French government as a common solution for national research institutes. This instance shows some differences with your test Dataverse instance. I left some integration tests unchanged, as they fail on my side but should work in your current environment.

Have a nice day,
Louis Tromel.

Ash84 and others added 6 commits January 4, 2023 16:50
…the metadata which is no more only limited to the metadata field Citation.

Functions of interaction with the Dataset added: new metadata fields (GeographicBoundingBox, KindOfData, DatasetTimePeriodCovered). Error return added in the implemented functions that interact with the remote server. UsersOperations added to interact with the Dataverse token. Added enumerations for fields that have the "controlledVocabulary" attribute and were missing one. FileUploader functions are now inside SwordAPI.
# Conflicts:
#	.gitignore
#	build.gradle
#	src/integration-test/java/com/researchspace/dataverse/http/AbstractIntegrationTest.java
#	src/integration-test/java/com/researchspace/dataverse/http/DatasetOperationsTest.java
#	src/integration-test/java/com/researchspace/dataverse/http/DataverseOperationsTest.java
#	src/integration-test/java/com/researchspace/dataverse/http/MetadataOperationsTest.java
#	src/main/java/com/researchspace/dataverse/api/v1/DatasetOperations.java
#	src/main/java/com/researchspace/dataverse/entities/facade/DatasetFacade.java
#	src/main/java/com/researchspace/dataverse/http/AbstractOpsImplV1.java
#	src/main/java/com/researchspace/dataverse/http/DataverseOperationsImplV1.java
#	src/main/java/com/researchspace/dataverse/search/entities/SearchConfig.java
#	src/main/java/com/researchspace/dataverse/sword/FileUploader.java
#	src/test/java/com/researchspace/dataverse/entities/facade/DatasetTestFactory.java
#	src/test/java/com/researchspace/dataverse/search/SearchConfigTest.java
@otter606
Copy link
Contributor

otter606 commented Mar 2, 2023

Hi Louis
Many thanks for this great contribution! I will review this over next few days, probably at the weekend.
The test instance used for integration tests is the official Dataverse demo instance, so it's important that tests still pass on that.
There look to be quite a few breaking API changes - renaming/deleting methods, altering return types etc. It would be good to avoid doing this -e.g by adding in new methods so that old ones keep working.
There's been an other suggestion recently to make it easier to customize the brhaviour to support different Dataverse configurations. I'll have a think on how to achieve this so that customisations can be made by subclassing/configuring rather than changes to the core APIs.
Thanks very much
Richard

/**
* Get token expiration date.
* @return java.util.Date token expiration date.
* @return java.time.LocalDateTime token expiration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break existing client code that expects java.util.Date. To avoid doing that, suggest creating a new method with a variant name e.g. 'getTokenExpirationLocalDateTime'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a work in progress, and is not breaking anything because the method was non existing before.

* @throws ParseException
* @throws RestClientException
*/
String recreateToken();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleting /renaming this will break existing clients. Solution is to keep the original method signature and add your method as a new method - it looks like it is doing something different?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method was finally not developed. We considered, that in our use, it was better to create the token ourselves when it would expire.

@Ash84
Copy link
Author

Ash84 commented Mar 14, 2023

Hi again,
I'll try the tests with the official Dataverse demo when I have time, and repair those which are broken.
There should be as few changes in public classes as possible to avoid compatibility issues, but we needed to expand the scope of some core classes.
I understand your comments though, I wish I had more time to answer your questions but I can't deal with them right now. I will next week hopefully, and then I will make improvements in the following weeks to meet your requirements.
Thanks !

@Ash84
Copy link
Author

Ash84 commented Apr 26, 2023

Hi Louis Many thanks for this great contribution! I will review this over next few days, probably at the weekend. The test instance used for integration tests is the official Dataverse demo instance, so it's important that tests still pass on that. There look to be quite a few breaking API changes - renaming/deleting methods, altering return types etc. It would be good to avoid doing this -e.g by adding in new methods so that old ones keep working. There's been an other suggestion recently to make it easier to customize the brhaviour to support different Dataverse configurations. I'll have a think on how to achieve this so that customisations can be made by subclassing/configuring rather than changes to the core APIs. Thanks very much Richard

Hello,
Everything should now work well. I'm also thinking about a good way to make the different configurations work together. Some fields like "kindOfData" or "productionPlace" don't work the same way between https://demo.dataverse.org/ and https://demo.recherche.data.gouv.fr/ . I agree with you, it is necessary to allow a more precise configuration of the expected fields according to the Dataverse with which one interacts.
Unfortunately I don't have time to make the changes less or non API-breaking, but when I can, I will spend a some to reduce the number of API changes. As I recall I made a point of changing as few public methods as possible, I'll have to do some comparison and testing.
In the meantime, I still suggest my improvements and some test fixes that might interest you.
I wish you a good day,
Louis

@pdurbin
Copy link
Member

pdurbin commented Apr 26, 2023

productionPlace changed in Dataverse 5.13:

I hope that backward-incompatibilities were not introduced but it's certainly possible. 😅 😬

@Ash84
Copy link
Author

Ash84 commented May 15, 2023

productionPlace changed in Dataverse 5.13:

* [Make productionPlace multiple, facetable, and enabled for Advanced Search #9253 dataverse#9254](https://github.com/IQSS/dataverse/pull/9254)

I hope that backward-incompatibilities were not introduced but it's certainly possible. sweat_smile grimacing

Hello,
I have taken this evolution into account, and the pull request includes the required implementations.

@Ash84 Ash84 marked this pull request as draft August 8, 2024 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants