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

Add NamespaceMapper #388

Closed
wants to merge 12 commits into from

Conversation

@DavidLi119
Copy link

commented Mar 6, 2019

issue#300
Created NamespaceMapper.java based on NamespaceApiMapper with changes from epic of issue#301. Reverted NamespaceApiMapper back to previous version due to accidentally changing it.

DavidLi119 added 2 commits Mar 6, 2019
Update NamespaceApiMapper.java
Added `final` to the class declaration, a private constructor and `static` to functions with `map` before the parameters. Please advise me if incorrect.
add NamespaceMapper
issue#300
Created NamespaceMapper.java based on NamespaceApiMapper with changes from epic of issue#301. Reverted NamespaceApiMapper back to previous version due to accidentally changing it.
public final class NamespaceMapper extends Mapper<NamespaceResponse, Namespace> {
private NamespaceMapper() {}

public static Namespace map(NamespaceResponse namespace) {

This comment has been minimized.

Copy link
@wslulciuc

wslulciuc Mar 6, 2019

Member

This class should only convert a NamespaceRequest to a Namespace. So, we'll want to remove this method.

import marquez.api.models.NamespaceResponse;
import marquez.service.models.Namespace;

public final class NamespaceMapper extends Mapper<NamespaceResponse, Namespace> {

This comment has been minimized.

Copy link
@wslulciuc

wslulciuc Mar 6, 2019

Member

Mapper is a legacy class that will be deprecated soon. Here's the related issue: #389. Meaning, we can define the class without extending the mapper class

null, namespace.getName().toLowerCase(), namespace.getOwner(), namespace.getDescription());
}

public Namespace of(String namespaceName, NamespaceRequest request) {

This comment has been minimized.

Copy link
@wslulciuc

wslulciuc Mar 6, 2019

Member

Nice! So, since you copied over the code from NamespaceApiMapper. Let's make sure we update and name all converter methods as map().

Here's what I'd recommend for your map() method:

This comment has been minimized.

Copy link
@DavidLi119

DavidLi119 Mar 6, 2019

Author

Ok, so gathering what you said and referring to DbTableVersionMapper.java, I made the following changes:

  1. Removed the extends Mapper<>
  2. Added @NonNull before all method args
  3. changed public Namespace of() to public static Namespace map()
  4. Commented out the first method
public final class NamespaceMapper {
  private NamespaceMapper() {}

/*  public static Namespace map(NamespaceResponse namespace) {
    requireNonNull(namespace, "namespace must not be null");
    return new Namespace(
        null, namespace.getName().toLowerCase(), namespace.getOwner(), namespace.getDescription());
  }*/

  public static Namespace map(@NonNull NamespaceName namespaceName, @NonNull NamespaceRequest request) {
    return map(
        new NamespaceResponse(
            namespaceName, null, request.getOwner(), request.getDescription().orElse(null)));
  }
}

This comment has been minimized.

Copy link
@wslulciuc

wslulciuc Mar 7, 2019

Member

You're making great progress, @DavidLi119! I noticed the CI build is failing. Are you able to run the following cmd successfully locally on your machine?

$ ./gradlew compileJava
public final class NamespaceMapper {
private NamespaceMapper() {}

/* public static Namespace map(NamespaceResponse namespace) {

This comment has been minimized.

Copy link
@wslulciuc

wslulciuc Mar 7, 2019

Member

Let's remove this method.


public static Namespace map(@NonNull NamespaceName namespaceName, @NonNull NamespaceRequest request) {
return map(
new NamespaceResponse(

This comment has been minimized.

Copy link
@wslulciuc

wslulciuc Mar 7, 2019

Member

The return type for this method is Namespace, but you're instantiating a NamespaceResponse. Mind double checking the logic of the method to ensure we are returning the correct class type?

@DavidLi119

This comment has been minimized.

Copy link
Author

commented Mar 7, 2019

So I removed the constructor like you said, though the reason I put it there was because DbTableVersionMapper.java had it. I also made it more similar to DbTableVersionMapper.java in the sense that it returns a new Namespace, however comparing to the Namespace Constructor I have a few questions below:

public final class NamespaceMapper {
   public static Namespace map(@NonNull NamespaceName namespaceName, @NonNull 
      NamespaceRequest request) {
         return new Namespace(
            //UUID?
            namespaceName,
            request.getOwner(),
            request.getDescription().orElse(null)
            );
  }
}

Im looking at this Namespace constructor:

public Namespace(UUID guid, String name, String ownerName, String description) {
    this.guid = guid;
    this.name = name;
    this.ownerName = ownerName;
    this.description = description;
    this.createdAt = null;
  }

Also, I am unable to run the ./gradlew command. It says a JDK is missing from my Visual Studios Code

DavidLi119 added 2 commits Mar 7, 2019
@DavidLi119

This comment has been minimized.

Copy link
Author

commented Mar 7, 2019

So I installed gradle 5.2.1, however the command ./gradlew test still fails at TASK :compileJava

@wslulciuc

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Hey, @DavidLi119. Thanks for making the changes and sorry for the delayed review.

So I removed the constructor like you said, though the reason I put it there was because DbTableVersionMapper.java had it.

You were right to add a private constructor. Mappers are more like utility classes, so we don't allowing class instantiation (similar to class Math in Java). In my earlier comment to "remove this method", I was referring to Namespace map(NamespaceResponse) that was commented out. Mind adding the private constructor back?

Im looking at this Namespace constructor:

public Namespace(UUID guid, String name, String ownerName, String description) {
this.guid = guid;
this.name = name;
this.ownerName = ownerName;
this.description = description;
this.createdAt = null;
}

We are working on updating our service models to not contain UUIDs (see #363). We use UUIDs as the primary key for a row in our DB and don't want to expose them to the callers of our API.

In class Namespace, I recommend defining a new constructor:

public Namespace(String name, String ownerName, String description) {
...
}

That way you can avoid generating a UUID in the mapper and properly convert a NamespaceRequest to a Namespace.

Happy to clarify things further if needed!

@wslulciuc

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

So I installed gradle 5.2.1, however the command ./gradlew test still fails at TASK :compileJava

I'm sure you have, but wanted to ask if you have the prerequisites installed to run gradle? https://gradle.org/install/#prerequisites

@DavidLi119

This comment has been minimized.

Copy link
Author

commented Mar 13, 2019

Sorry for the late reply. So it turns out I have not updated my Java to the latest version for a very long time, thus .\gradlew test not working is obvious (solved).
Now I have a few questions I hope you can help me resolve:

  1. I added the new constructor into the Namespace class, however when I test it, it fails the test saying that the variable guid has not been initialized, however this is one of the variables you did not want to include. How should I proceed?
    Error:
    > Task :compileJava C:\Users\darky\Documents\GitHub\marquez\src\main\java\marquez\service\models\Namespace.java:53: error: variable guid might not have been initialized } ^
  2. With this constructor, you wanted the name to be type String, however in the NamespaceMapper.java, you wanted the name to be type NamespaceName. On another note, I had to add an import line so the test could actually find type NamespaceName. How do I proceed?
  3. A few minor questions about the code itself:
package marquez.api.mappers;

import static java.util.Objects.requireNonNull; \\necessary?
import lombok.NonNull;                       \\added otherwise `@NonNull` couldn't be found
import marquez.api.models.NamespaceRequest;
import marquez.api.models.NamespaceResponse;
import marquez.service.models.Namespace;
import marquez.common.models.NamespaceName; 
\\^added so type NamespaceName could be referenced

public final class NamespaceMapper {
  private NamespaceMapper() {}

  public static Namespace map(@NonNull NamespaceName namespaceName, @NonNull NamespaceRequest request) {
    return new Namespace(
            namespaceName,
            request.getOwner(),
            request.getDescription().orElse(null)
            );
  }
}

Lastly, a change to the .gitignore occurred and I do not know if it is a problem
/.metadata/ was added to the bottom

@@ -16,3 +16,4 @@ build/

# Marquez configuration
config.yml
/.metadata/

This comment has been minimized.

Copy link
@wslulciuc

wslulciuc Mar 18, 2019

Member

You'll want to define a .gitignore_global to ignore files specific to your dev environment. The .gitignore in this repo serves as excluding files specific to the project.

@wslulciuc

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

With this constructor, you wanted the name to be type String, however in the NamespaceMapper.java, you wanted the name to be type NamespaceName. On another note, I had to add an import line so the test could actually find type NamespaceName. How do I proceed?

Have the types be different is fine. The constructor for Namespace() takes a namespace as a string, which is what we want.

For your other questions, I left comments in your PR. They should help and set you on the right path to get this PR merged. Let me know if you need more clarity.

Thanks again for helping out, @DavidLi119!

@DavidLi119

This comment has been minimized.

Copy link
Author

commented Mar 18, 2019

@wslulciuc So apparently ./gradlew test has a problem with type NamespaceName. the error is:
Task :compileJava ..\marquez\src\main\java\marquez\api\mappers\NamespaceMapper.java:28: error: incompatible types: NamespaceName cannot be converted to String
So is there a problem with my NamespaceMapper.java or the model NamespaceName.java?

@wslulciuc

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

So is there a problem with my NamespaceMapper.java or the model NamespaceName.java?

You'll want to use NamespaceName,getValue() to return the string representation of the namespace name. That should resolve your compilation error.

DavidLi119 added 4 commits Mar 19, 2019
ran .gradlew spotlessApply
circleci failed test due to this
@DavidLi119

This comment has been minimized.

Copy link
Author

commented Mar 19, 2019

@wslulciuc The compilation error was fixed and all subsequent problems have been fixed. Only requiring codecov checks to be passed. Thanks for working with me through this issue. Been a pleasure.

@wslulciuc

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Great work, @DavidLi119! Since this PR is introducing new code, you will also need to write a test to meet our coverage requirements (see CONTRIBUTING.md). As an example, I would reference DbTableVersionMapperTest

@DavidLi119

This comment has been minimized.

Copy link
Author

commented Mar 19, 2019

@wslulciuc I can't seem to find the test coverage requirements on the CONTRIBUTING.md, can you link them to me? Also, is it possible to let me know how I should start? I do not have any experience writing tests.

@wslulciuc

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

We block PRs if codecoverage drops (i.e. codecov/patch, codecov/project). It's really in place to minimize the introduction of bugs. See step 5 in "Submitting a pull request"

@wslulciuc

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

I do not have any experience writing tests.

For a background on writing Java tests, see: https://junit.org/junit5. And for an example on how to write a test for NamespaceMapper, see DbTableVersionMapperTest

@wslulciuc

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Closes #300

@wslulciuc

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@DavidLi119: Were the links provided above helpful? Let me know If I can assist in anyway.

@wslulciuc wslulciuc added the review label Apr 14, 2019

@wslulciuc

This comment has been minimized.

Copy link
Member

commented May 9, 2019

@DavidLi119, We'll have to close this PR as it has been a while since we've seen activity. Please feel free to re-open.

@wslulciuc wslulciuc closed this May 9, 2019

@wslulciuc wslulciuc added in progress and removed review labels May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.