Added 'addUrl' method

[ghost]

fixes #714

[what-the-diff]

• Added a new method addUrl(Locale locale, URL url) to BaseFaker class.
• Modified ProviderRegistration interface by adding the same method as in 1).
• FakeValuesService was modified with addition of addUrl() and loadFromURL().
• FakeValuesTest was updated accordingly (added tests for 3.)

[snuyanzin]

@tcttk thanks for the contribution

1. Can you please explain how this change relates to custom providers?

2. And as I mentioned in Unable to create custom provider using yaml file when bundled as a jar. #714 before fixing it please provide a reproducible test case first

[codecov-commenter]

Codecov Report

Merging #727 (c9c20b5) into main (dd18928) will decrease coverage by 0.11%.
The diff coverage is 66.66%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main     #727      +/-   ##
============================================
- Coverage     92.88%   92.78%   -0.11%     
- Complexity     2583     2585       +2     
============================================
  Files           281      281              
  Lines          5114     5126      +12     
  Branches        528      530       +2     
============================================
+ Hits           4750     4756       +6     
- Misses          236      240       +4     
- Partials        128      130       +2     

Impacted Files	Coverage Δ
...n/java/net/datafaker/providers/base/BaseFaker.java	89.38% <0.00%> (-1.62%)	⬇️
...datafaker/providers/base/ProviderRegistration.java	100.00% <ø> (ø)
.../java/net/datafaker/service/FakeValuesService.java	84.16% <55.55%> (-0.15%)	⬇️
...rc/main/java/net/datafaker/service/FakeValues.java	81.96% <90.00%> (+0.30%)	⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[bodiam]

Thanks for this PR. Interesting to use a URL for this, how does this work? I expected something like an InputStream or so.

[ghost]

hi @bodiam @snuyanzin,

With this change, we can now add yaml files for the custom providers not only from the path, but also from the url. By giving a valid url parameter, it will get a stream from which you read the contents of the yaml and add to the map.

I coded this fix because I encountered the problem as described in #714. Path class throws an exception when trying to stream files from external libraries, URL class does not.

my-app (addPath(locale, Paths.get("src/main/resources/de/sample.yml")) works ✅)
|
|----> /resources/sample.yml  
|----> depends datafaker-library 

my-app
|
|----> depends my-datafaker-library (addPath(locale, Paths.get("src/main/resources/de/sample.yml")) does not work ❌)
                           |
                           |----> /resources/sample.yml  
                           |----> depends datafaker-library

my-app
|
|----> depends my-datafaker-library (addUrl(locale, getClass().getClassLoader().getResource(locale.getLanguage() + "/sample.yml")) works ✅)
                           |
                           |----> /resources/sample.yml  
                           |----> depends datafaker-library 

my-app
|
|----> depends my-datafaker-library (addUrl(locale, new URL("https://raw.githubusercontent.com/datafaker-net/datafaker/main/src/main/resources/de.yml")) works ✅)
                           |
                           |----> depends datafaker-library 

[snuyanzin]

@tcttk thanks for the explanation
well I put a code snippet working for the case you've mentioned

my-app
|
|----> depends my-datafaker-library (addPath(locale, Paths.get("src/main/resources/de/sample.yml")) does not work ❌)
                           |
                           |----> /resources/sample.yml  
                           |----> depends datafaker-library

#714 (comment)

it should work.

Regarding changes I'm still a bit skeptic about: there is no any test confirming that it works.
And it means there is no any guarantee that it will keep working while merging other changes

[ghost]

In the example you gave, I think the yaml file is in the project. What I'm talking about is this occurs when the yaml file is in a different jar.

The error I get when I test the case above is
java.lang.IllegalArgumentException: Path should be an existing readable file.

You can check out the code block here.
https://github.com/datafaker-net/datafaker/blob/main/src/main/java/net/datafaker/service/FakeValuesService.java#L94

[snuyanzin]

ok i see... loading files from other jars looks strange for me...
anyway

The error I get when I test the case above is
java.lang.IllegalArgumentException: Path should be an existing readable file.

to fix it the condition could be tuned like

if (path == null || Files.isDirectory(path)) {
            throw new IllegalArgumentException("Path should be an existing file");
        }

and no need for a new methods and so on.
the only missing thing still absence of a specific test for that in tests, however now even without it already 90% is covered by existing tests

[ghost]

After changing condition, the code here started to give the error below.

SEVERE: Exception: java.nio.file.NoSuchFileException

That's why I added a new method with url parameter.

[snuyanzin]

Can you share the code giving an error?

I tested the code from #714 (comment)

and it works ok

[ghost]

I made the following edits.

• FakeValuesService

        if (path == null) {
            throw new IllegalArgumentException("Path should be an existing readable file");
        }

• FakeValues

        if (filePath == null) {
            return null;
        }

Then try to get the yaml file from an external jar you depend on as you mentioned here.

[snuyanzin]

and what it is the trace in case it is failing?
to be honest it is not reproduced on my machine....
that's another point to have a dedicated test for this in repo

[ghost]

I realize that this case is difficult to test. I will write the things to do to test it in order.

1. I created a project with datafaker dependency added. (named my-datafaker-library)
2. In this project, I wrote a class that extends Faker. We can use the code below.

public class MyAppFaker extends Faker {

    public MyAppFaker() {
        super();
    }

    public InsectFromResources insectFromResources() {
        return getProvider(InsectFromResources.class, InsectFromResources::new, this);
    }
}

3. In this project, I wrote a class that extends AbstractProvider. We can use the code below.

  public class InsectFromResources extends AbstractProvider<BaseProviders> {
        private static final String KEY = "insectsfromfile";

        public InsectFromResources(BaseProviders faker) {
            super(faker);
            faker.addPath(Locale.ENGLISH, Paths.get(this.getClass().getClassLoader().getResource("en/ants-resources.yml").getPath()));
            faker.addPath(Locale.ENGLISH, Paths.get(this.getClass().getClassLoader().getResource("en/bees-resources.yml").getPath()));
        }

        public String ant() {
            return resolve(KEY + ".ants");
        }

        public String bee() {
            return resolve(KEY + ".bees");
        }
    }

4. I added the files mentioned in the class into the language folder. (e.g. /src/main/resources/en/ants-resources.yml)
5. I packaged my-datafaker-library project as jar.
6. I created a project with my-datafaker-library dependency added. (named my-app)
7. I wrote a test method as follows.

    @Test
    void myAppFaker() {
        MyAppFaker myAppFaker = new MyAppFaker();
        System.out.println(myAppFaker.insectFromResources().ant());
        System.out.println(myAppFaker.insectFromResources().bee());
    }

8. I got the error when I tested it.

[snuyanzin]

Thanks for your explanation @tcttk
Finally I was able to reproduce it...
From one side it's not trivial to create an integration test for this,
from another side I added one more commit to this PR where I made addPath working via addUrl so most of the time the code is reused and at covered by tests for old addPath.
Please double check if it keeps working for your use case and let us know if you are ok with such changes.

[ghost]

Hey @snuyanzin,
I reviewed your commit and tested my scenario. It works ✌️ thank you for your support.

Which version are you planning to deploy with?

[snuyanzin]

Great, thanks for checking.

About version, i guess it should be something like 2.x