Skip to content
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

[Feature] Add xquery recursive collection creation with just one path expression #5062

Open
wants to merge 25 commits into
base: develop
Choose a base branch
from

Conversation

enima007
Copy link

@enima007 enima007 commented Sep 17, 2023

Description:

Added an xmldb:create-collection#1 function to XMLDBModule.java and updated XMLDBCreateCollection.java implementation
to allow the creation of a new collection just by specifying its path.

Reference:

Closes #4238.

Type of tests:

Added a small assertEqual test in exist-core/src/test/xquery/xmldb/collection-create-tests.xql to check if
the collection is created successfully.

@dizzzz dizzzz changed the title Feature/add xquery collection create with just the path [Feature] Add xquery recursive collection creation with just one path expression Sep 19, 2023
Copy link
Contributor

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

Excellent first contribution. Just a few things that can be tidied up.

} else {
try {
return new StringValue(this, collection.getName());
} catch (XMLDBException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

exception should be marked final

if (collectionNeedsClose && collection != null) {
try {
collection.close();
} catch (final Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid catch-all Exception. Instead specify explicitly multiple exceptions separated by |

Copy link
Member

Choose a reason for hiding this comment

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

code has gone?

Copy link
Contributor

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

Paths to xmldb:create-collection#1 must be absolute.

declare
%test:assertEquals("/db/parent-collection/path/to/new-collection")
function t:fnCreateNewRecursiveCollection() {
let $collection := xmldb:create-collection($t:path-collection)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that you are currently testing that:

xmldb:create-collection("/parent-collection/path/to/new-collection")

creates the collection /db/parent-collection/path/to/new-collection?

If so, that should not work! The xmldb:create-collection#1 function should always take an absolute path, i.e. starting with /db. This test should be modified to check for an error result, and the implementation updated accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

looks good to me

@line-o line-o requested a review from a team October 23, 2023 17:46
@sonarcloud
Copy link

sonarcloud bot commented Oct 23, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

48.0% 48.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

Copy link
Member

@dizzzz dizzzz left a comment

Choose a reason for hiding this comment

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

LGTM

}

try {
final Collection newCollection = createCollectionPath(collection, collectionName);
Copy link
Member

Choose a reason for hiding this comment

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

shoudl this go in to try(final Collection newCollection..) ?

@line-o line-o added enhancement new features, suggestions, etc. awaiting-response requires additional information from submitter labels May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-response requires additional information from submitter enhancement new features, suggestions, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] add mkdir -p like function to xmldb XQuery module
4 participants