sockeqwe / fragmentargs

Annotation Processor for setting arguments in android fragments
http://hannesdorfmann.com/android/fragmentargs
Apache License 2.0
1.08k stars 87 forks source link

@Arg(required = false) throws NullPointerException on latest 4.0.0-SNAPSHOT #72

Closed fmnstr closed 8 years ago

fmnstr commented 8 years ago

I tried the latest version 4.0.0-SNAPSHOT but my arguments annotated with required = false @Nullable @Arg(required = false) String mId throw NullPointerException as you can see on the compiled builder class if (id == null) { throw new NullPointerException("Argument 'mId' must not be null."); }

sockeqwe commented 8 years ago

Could you please share the generated Builder code? How do you use the builder? builder.id(null)?

fmnstr commented 8 years ago

Generated builder class:

package eu.afse.omnia.prototype.usecases.transfers.transfers.activetransfers;

import android.os.Bundle;
import android.support.annotation.NonNull;

public final class ActiveTransfersFragmentBuilder {

  private final Bundle mArguments = new Bundle();

  public ActiveTransfersFragmentBuilder(@NonNull String type) {

    if (type == null) {
      throw new NullPointerException("Argument 'type' must not be null.");
    }
    mArguments.putString("type", type);
  }

  @NonNull
  public static ActiveTransfersFragment newActiveTransfersFragment(@NonNull String type) {
    return new ActiveTransfersFragmentBuilder(type).build();
  }

  public ActiveTransfersFragmentBuilder selectedActiveTransferFilter(@NonNull com.afse.aibas.ebanking.direct.core.domain.object.activeTransfers.ActiveTransferFilter selectedActiveTransferFilter) {

    if (selectedActiveTransferFilter == null) {
      throw new NullPointerException("Argument 'mSelectedActiveTransferFilter' must not be null.");
    }
    mArguments.putParcelable("selectedActiveTransferFilter", selectedActiveTransferFilter);
    return this;
  }

  public ActiveTransfersFragmentBuilder selectedTransactionId(@NonNull String selectedTransactionId) {

    if (selectedTransactionId == null) {
      throw new NullPointerException("Argument 'mSelectedTransactionId' must not be null.");
    }
    mArguments.putString("selectedTransactionId", selectedTransactionId);
    return this;
  }

  public Bundle buildBundle() {
    return new Bundle(mArguments);
  }

  public static final void injectArguments(@NonNull ActiveTransfersFragment fragment) {
    Bundle args = fragment.getArguments();
    if (args == null) {
      throw new IllegalStateException("No arguments set. Have you setup this Fragment with the corresponding FragmentArgs Builder? ");
    }

    if (args != null && args.containsKey("selectedTransactionId")) {
      fragment.mSelectedTransactionId = args.getString("selectedTransactionId");
    }

    if (args != null && args.containsKey("selectedActiveTransferFilter")) {
      fragment.mSelectedActiveTransferFilter = args.getParcelable("selectedActiveTransferFilter");
    }

    if (!args.containsKey("type")) {
      throw new IllegalStateException("required argument type is not set");
    }
    fragment.type = args.getString("type");
  }

  @NonNull
  public ActiveTransfersFragment build() {
    ActiveTransfersFragment fragment = new ActiveTransfersFragment();
    fragment.setArguments(mArguments);
    return fragment;
  }

  @NonNull
  public <F extends ActiveTransfersFragment> F build(@NonNull F fragment) {
    fragment.setArguments(mArguments);
    return fragment;
  }
}

Code arguments: @Nullable @Arg(required = false) String mSelectedTransactionId; @Nullable @Arg(required = false) ActiveTransferFilter mSelectedActiveTransferFilter; @Arg String type;

new instance call with exception: return new ActiveTransfersFragmentBuilder(ActiveTransfersFragment.ACTIVE_TRANSFERS_PAGE_TRANSFERS).selectedTransactionId(mSelectedId).build();

This code works in library version 3.0.2

sockeqwe commented 8 years ago

Yep, that nullcheck feature has been introduced in 4.0 with #45 . We wanted to crash early to find a better way to track back where the null value comes from (directly when creating the fragment via builder instead of Nullpointer exception when accessing the variable for the first time)

The idea was that if a value is optional like mSelectedActiveTransferFilter in your case, you are not allowed to call builder.selectedTransactionId(null).

 ActiveTransfersFragmentBuilder  builder = new ActiveTransfersFragmentBuilder(ActiveTransfersFragment.ACTIVE_TRANSFERS_PAGE_TRANSFERS);

if (mSelectedId != null){
builder.selectedTransactionId(mSelectedId);
}
return builder.build();

So, this is working as designed ... We could discuss whether or not this is a good design decision ...

fmnstr commented 8 years ago

Hmm. Ok I understand this point. However, think of the alternative. On a big code base, consideration should be taken, (against testing or code proofing every argument passed), that not null arguments will be passed to the builder, providing alternate builder calls in case it did. That makes this update a little bit harder. I guess that's why the number is 4.0.0 and not 3.x . :) In your defense I see that you had already annotated your builder methods with @NonNull from 3.0.2 so we should have taken that into consideration . :)

sockeqwe commented 8 years ago

Yes ... as already said, this api is not fixed and I'm open to discuss that