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 immutable entity operator #73

Closed

Conversation

wreulicke
Copy link
Contributor

@wreulicke wreulicke commented Jan 31, 2019

Implements #74.

This PR provides to support immutable entity.

Below entity code works due to this PR.

public class Subscription {

 	@PrimaryKey(generationType=GenerationType.APPLICATION)
	public final Long int userId;

	public UserInfo(@Column(name = "userId") Long userId) {
		this.userId = userId;
	}
}

How do you think of this PR?

This PR contains potentially breaking changes...

@wreulicke wreulicke changed the title add immutable entity operator Add immutable entity operator Jan 31, 2019
@wreulicke wreulicke changed the title Add immutable entity operator [WIP] Add immutable entity operator Jan 31, 2019
@wreulicke wreulicke changed the title [WIP] Add immutable entity operator Add immutable entity operator Jan 31, 2019
@aadrian
Copy link
Member

aadrian commented Feb 3, 2019

@wreulicke why does it have to be breakable?

@takezoe
Copy link
Member

takezoe commented Feb 4, 2019

@aadrian What does "breakable" mean?

@wreulicke
Copy link
Contributor Author

wreulicke commented Feb 4, 2019

@aadrian
This diff into DefaultPropertyExtractor is related

this PR changes behavior with codes like below.

public class Subscription {
    // this property has getter/setter
    private String property.

    // this property has no getter/setter.
    private String transientProperty
}

The transientProperty in above codes will be recognized/extracted as property by this PR if use DefaultPropertyOperator.
But before, not recognized/extracted.
If recognized/extracted as property, mirage will persist it as column.
(This will cause a SQLException.)

So, I say "This PR contains potentially breaking changes..."

@takezoe
Copy link
Member

takezoe commented Feb 5, 2019

This PR contains potentially breaking changes...

Ah, I overlooked this comment in the pull request. I think the immutable entity is good. But we need a way to keep the current behavior in case users need it.

By the way, userId in the first example should be final to be immutable like below?

public class Subscription {

 	@PrimaryKey(generationType=GenerationType.APPLICATION)
	public final Long userId;

	public UserInfo(@Column(name = "userId") Long userId) {
		this.userId = userId;
	}
}

@wreulicke
Copy link
Contributor Author

By the way, userId in the first example should be final to be immutable like below?

great! fix it.

@wreulicke
Copy link
Contributor Author

Ah, I overlooked this comment in the pull request. I think the immutable entity is good. But we need a way to keep the current behavior in case users need it.

Thanks for your review. I agree with you.

I close this PR now, and i will send PR to add test for keep compatibility.
Please review if recreate PR for immutable entity, Thanks.

@wreulicke wreulicke closed this Feb 5, 2019
@aadrian
Copy link
Member

aadrian commented Feb 5, 2019

@wreulicke also please consider the Map support, since this is also a very important feature.

@wreulicke
Copy link
Contributor Author

also please consider the Map support, since this is also a very important feature.

@aadrian Thanks for your review.
I will consider also.

By the way, Why do you think that the Map support is important?
For Clojure? 😆

@aadrian
Copy link
Member

aadrian commented Feb 6, 2019

@wreulicke yes indeed, for Clojure too, for Groovy, and also for dynamic tooling (like ETL) that can't use classes, since stuff needs to be dynamic.

@wreulicke wreulicke deleted the feature/add-immutable-operator branch February 6, 2019 08:53
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